environment management overhaul #62
Conversation
repository/util.go
Outdated
| return homedir.Expand(filepath.Join(cuRepoPath, normalizedOrigin)) | ||
| } | ||
|
|
||
| func normalizeGitURL(endpoint string) (string, error) { |
There was a problem hiding this comment.
This boilerplate was copied from dagger, which in turn was copied from Go.
It simply normalizes e.g. https://github.com/dagger/container-use.git and [email protected]:dagger/container-use.git both to github.com/dagger/container-use
aluzzardi
force-pushed
the
env-management
branch
5 times, most recently
from
a5a003f to
87775df
Compare
aluzzardi
force-pushed
the
env-management
branch
from
87775df to
7c35e0c
Compare
Signed-off-by: Andrea Luzzardi <[email protected]>
aluzzardi
force-pushed
the
env-management
branch
from
7c35e0c to
d5a7b95
Compare
There was a problem hiding this comment.
directionally this looks right to me, accomplishes the more immediate goals of #59 without trying to dot every i and cross every t.
imo we need to get this and #65 onto main ASAP to clear a path for more incremental work... eg i wanna get the "real" #11 environment resume working. with this PR plus the tests, the impl will be much cleaner (although it'll still involve similar dirty tricks with worktree .git pointers)
parts of my Remote refactor will fit in here as a sub-package of Repository, i think.
environment/environment.go
Outdated
| func (env *Environment) Export(ctx context.Context) (rerr error) { | ||
| _, err := env.container.Directory(env.Config.Workdir).Export( | ||
| ctx, | ||
| env.Worktree, |
There was a problem hiding this comment.
this is a bit of a hole in the abstraction. not a problem for now but a good thing to noodle on.
There was a problem hiding this comment.
Agreed.
I was thinking it's not that bad because the interface of environments is pretty straightforward: worktree in (New, Load), worktree out (Export). The rest is out of scope for the environment.
There was a problem hiding this comment.
the hard part, and a big part of why #59 felt like boiling the ocean, is eventually it'd great to point these at a network remote and these lil abstraction violations will prevent us from doing that.
There was a problem hiding this comment.
Yeah. I actually kept #59 in mind while doing this -- I was kinda hoping that this would be compatible. 1) Keep the environment dumb (load stuff , dump stuff ). 2) All that logic moved as-is (for now) in a layer above 3) Eventually Repository could use the Remote abstraction (e.g. rather than integrating Remote in all of Environment, we could integrate it into Repository only?)
Just realized that maybe I'm missing something. I assumed we'd still use local storage as a staging area, even for other storage implementation, but I see now you meant to be able to avoid doing that and going straight to the alternative storage without stopping by local disk?
There was a problem hiding this comment.
I assumed we'd still use local storage as a staging area, even for other storage implementation, but I see now you meant to be able to avoid doing that and going straight to the alternative storage without stopping by local disk?
yep, that's the eventual goal. honestly it's very cart-before-horse to be trying to break your assumption right now, but that's where I started with #59. We'll get there eventually, but we also need to upstream some Git apis into dagger first anyways.
Signed-off-by: Andrea Luzzardi <[email protected]>
aluzzardi
force-pushed
the
env-management
branch
from
094d2a8 to
654aee6
Compare
Signed-off-by: Andrea Luzzardi <[email protected]>
Signed-off-by: Andrea Luzzardi <[email protected]>
| ), | ||
| mcp.WithString("source", | ||
| mcp.WithString("environment_source", |
There was a problem hiding this comment.
one day we're gonna have evals so we can make these changes on more than just vibes XD
| if resp, err := updateRepo(); err != nil { | ||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
noting here that in the case where we fail to update the repo, we discard runErr.
i think that's fine for now, in theory the "failed to update repo" is the more violent and important error. it may be hard to debug without logs from environment.Run, but that failure mode is also mostly theoretical for right now.
same thing exists below for the !background case.
There was a problem hiding this comment.
Yeah went back and forth between the 2. My thinking was that a failed command "happens", it's just the model iterating. However, failing to update the repo is an urgent error that needs to be brought forward to the user (something is very wrong)
This overhauls how state and environments are managed.
Repositorycontains CRUD operations around environments