Repository navigation
workspace: compare changes from an explicit baseline - #13855
Conversation
1afe7c0 to
36c178e
Compare
36c178e to
45d1352
Compare
grouville
changed the title
grouville
changed the title
eadeb2c to
b920a58
Compare
vito
left a comment
vito
left a comment
There was a problem hiding this comment.
Just a couple questions, approving pending their resolution (maybe no-ops)
| applied, err = handleWorkspaceResponse(ctx, dag, ws.WithChanges(changes), autoApply) | ||
| // Migration can hoist configuration from a nested module to the workspace | ||
| // root, so its preview and apply are deliberately root-scoped. | ||
| applied, err = handleWorkspaceResponse(ctx, dag, ws.WithWorkdir("."), ws.WithChanges(changes).WithWorkdir("."), autoApply) |
There was a problem hiding this comment.
What does withWorkdir(".") do? Seems like it would be a no-op - should it be /?
There was a problem hiding this comment.
Good question, I had to double-check too. Here withWorkdir(".") is not a no-op: Workspace paths are relative to the root, so . selects the root and / is invalid.
We need it on the updated workspace because migration can write dagger.toml above the current directory, while changes() returns paths from that directory. But you were right that it was redundant on from, so I removed it and added a comment in 3e35ffada.
| isEmpty, err := updated.Changes().IsEmpty(ctx) | ||
| // Installing a module edits the workspace config, which may live above the | ||
| // caller's cwd. Measure from the workspace root for this root-scoped check. | ||
| isEmpty, err := updated.WithWorkdir(".").Changes(dagger.WorkspaceChangesOpts{From: target.WithWorkdir(".")}).IsEmpty(ctx) |
There was a problem hiding this comment.
same question here - https://github.com/dagger/dagger/pull/13855/changes#r3785988145
There was a problem hiding this comment.
Same here. Installing a module can update the workspace config above the current directory, so we need withWorkdir(".") on updated. We do not need it on target, so I removed that extra call in 3e35ffada.
|
|
||
| // TestEngineVersionLatestStaysFloating covers the floating "latest" | ||
| // engineVersion surviving config edits. A dagger.json can declare | ||
| // "engineVersion": "latest" — module init writes exactly that — and the |
There was a problem hiding this comment.
I thought specifying "engineVersion": "latest" was just AI slop, and that all things writing it should just switch to using the current engine version?
But I may have been corrected on that and just forgotten. 😅 I just distinctly remember pointing it out in a call in the early days of one of the SDKs because the model that authored it didn't know there was an API to get the current engine version. Is it a valid use case after all?
There was a problem hiding this comment.
Yep, you were right. This was our mistake. latest is accepted as input, but when writing the config we should resolve it and pin the concrete engine version.
I removed the floating behavior and fixed module initialization in c773db368. Tests now cover both initialization and later config edits.
e0713bc to
3e35ffa
Compare
Workspace.changes previously returned every pending edit, including changes that were already present when a generator received the workspace. Applying that result could duplicate those earlier changes. New callers can pass the workspace they started from. The engine compares the two immutable workspace states and returns only the later edits, with paths relative to the current workspace cwd for modules using the new behavior. The argument remains optional so beta.9 CLIs and SDKs keep their cumulative behavior during the migration. Internal callers use the explicit baseline, and tests cover both forms. Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>
Add ModuleSource.generate(workspace), which returns the supplied Workspace with the module's generated files applied. The input workspace stays immutable, so an SDK can compare the result with the workspace it received and return only its own edits. This also composes across managed modules: each resulting workspace can be passed to the next module, and the SDK calls changes(from:) once at the end. Local dependencies are generated only as input to their parent, so their files are not accidentally returned twice. Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>
Regenerate the checked-in clients and API reference for the optional Workspace.changes(from:) baseline and ModuleSource.generate returning a Workspace. The existing asSDK.modules object shape remains unchanged. Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>
Go bindings represent optional GraphQL arguments with an options struct. Update the generator fixtures to pass the baseline workspace through WorkspaceChangesOpts so the generated fixture modules compile. Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>
These commands can update the workspace config above the caller's current directory. Keep the resulting workspace rooted so changes can include that file, but leave the comparison workspace unchanged because only its contents are used as the baseline. Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>
3e35ffa to
feca094
Compare
The engine applies a generator's changeset relative to the client's cwd, so a workspace-rooted changeset lands every path under the cwd a second time and leaves the module ungenerated. This SDK re-rooted its own result to compensate, and raised for a module found above the cwd, because the changesets came back workspace-rooted. dagger/dagger#13855 moved that into the engine: from v1.0.0-beta.10 a Workspace.changes result is measured from the workspace's own cwd, the caller supplies the baseline to compare against, and a change that falls outside the cwd fails loudly instead of being dropped. Declare the version and hand the work back — stage onto the workspace value and diff it against the baseline already in hand. The baseline is the dependency-staged workspace, not the one handed in: the dependencies' generated code belongs to their own SDKs and must stay out of this changeset, which is what the filtered before-directory used to express. Signed-off-by: Yves Brissaud <yves@dagger.io>
* Drop the polyfill for native workspace APIs The TypeScript SDK depended on `dagger/polyfill` for three things: selecting the modules it manages, forking the workspace to stage edits, and re-rooting the resulting changeset into the client's cwd. All three are now native in the engine (dagger/dagger#13854, dagger/dagger#13855), so the dependency goes. Signed-off-by: Tom Chauveau <tom@dagger.io>
2 participants
This completes the engine API needed to remove
dagger/polyfill.The caller now keeps the workspace it received, generates files on a new immutable workspace value, then compares the two:
ModuleSource.generate(workspace)returns aWorkspace, so SDKs can compose generation with normal workspace edits.Workspace.changes(from:)performs the comparison only when aChangesetis required and returns paths relative to the workspace cwd.This avoids a separate
WorkspaceDifftype and avoids afork()lifecycle that callers could forget. The baseline is an ordinary immutable workspace value already available to the caller. It also handles module initialization correctly: the SDK sees the staged module scaffold, but returns only the files it generated.ModuleSource.generateLocalDependenciesremains temporarily because released SDKs still call it. New SDK code usesgenerate; the compatibility field can be removed after those migrations ship.This PR builds on #13854.
Test