diff --git a/AGENTS.md b/AGENTS.md index d3d44f409..a371f6c79 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,7 +24,7 @@ Use Taskfile tasks rather than raw Go commands: - Tests use `testify/assert` for assertions - Test files follow `*_test.go` naming convention - If DR_API_TOKEN is set, run smoke tests: `task smoke-test` (but ask for permission before using a real API token) -- Workload/artifact acceptance (live): `task smoke-test-workload` for scenarios A–C + artifact, or `task smoke-test-workload-full` to include the ~20-30 min built-workload scenario D. Uses the CLI's existing `drconfig.yaml` auth (set `DATAROBOT_API_TOKEN` / `DATAROBOT_ENDPOINT` only to override); CI wiring is pending team review. +- Workload/artifact acceptance (live): `task smoke-test-workload` for scenarios A–C, E + artifact, or `task smoke-test-workload-full` to include the ~20-30 min built-workload scenario D. Uses the CLI's existing `drconfig.yaml` auth (set `DATAROBOT_API_TOKEN` / `DATAROBOT_ENDPOINT` only to override); CI wiring is pending team review. **Go Version Requirement:** Tests run with the `-race` flag for data race detection. The race runtime must match your Go compiler version exactly. If you see errors like `compile: version "go1.X.Y" does not match go tool version "go1.X.Z"`, ensure your installed Go version matches the version in `go.mod` (run `brew upgrade go` or adjust `go.mod` accordingly). diff --git a/CHANGELOG.md b/CHANGELOG.md index 4062dafe3..444caf0ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,11 +13,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - `dr workload config` over an existing manifest now names the `.env` variables the file does not declare, and the flag that adds them. Names only, never values, matching what the deploy plan already redacts. Silent when there is no `.env`, which is the ordinary CI case, and under `--skip-env`. Under `--output-format json` the wizard is given no stderr at all, so `2>&1 | jq .` parses; the one thing that would otherwise be lost, a `.env` that could not be read, becomes an error there rather than a warning nobody sees. It also names a declared variable whose `.env` value no longer matches the manifest, and the variables stored as credentials, whose values cannot be compared at all: saying so outright beats a silence that reads as "those are fine" while a rotated key sits unused. And it names what was held back rather than added: a variable the classifier read as local-only, and an entry left naming the credential placeholder by a store that could not be reached, which a later import skips because the name counts as declared. - `dr workload status`, `dr workload get`, `dr workload endpoint` and `dr workload logs` now take the workload id as an optional argument. Left out, it is read from the `workloadId` in the nearest `.datarobot.yaml`, searched upward from the new `--dir` flag (the current directory by default), so the commands `dr workload up` points you at can be run as printed from the project it just deployed. A typed id still wins, and is used without reading any manifest. The workload that was picked is named on stderr, except under `--output-format json`, where anything on stderr would break `2>&1 | jq .`. - `dr workload stop`, `dr workload start` and `dr workload delete` take the workload id optionally too, on the same terms. Because they change something, a workload whose id is specified in the manifest rather than on the command line is confirmed first; a typed id is never questioned. `--yes` skips the question, and `stop` and `start` gained that flag for this. `DATAROBOT_CLI_NON_INTERACTIVE=1` also skips it on `stop` and `start`, but not on a `delete` whose id came from the manifest: that variable is set once across a pipeline, and deleting something nobody named is not what it was set for. +- `dr workload up --recreate`: delete the bound workload and create it again under the same name, which is the recovery an errored or terminated workload needs, folded into the deploy that was already asked for. It acts only on those two states — a workload a deploy can act on is deployed onto, not replaced — asks for the name to be typed back unless `--yes` is passed, and under `--dry-run` deletes nothing while still planning against what would follow the delete, so the plan and the JSON action describe the run that would happen rather than the dead workload it starts from. +- `dr artifact code init --force`: re-point an already-linked directory at a different artifact instead of refusing. Before this, changing a link meant deleting `.datarobot/workload/`, which discards the code catalog, the last-synced version and the project's sync history for the sake of one field. The catalog comes from the artifact named on the command line, as it does for a first `init`, and the id being replaced is printed — as `previousArtifactId` under `--output-format json` — so a mistake can be undone. Passing the artifact the directory is already linked to is a no-op rather than a rewrite: the baseline reset that makes a real re-point correct would, against the same artifact, cost every local edit not yet synced. - `dr workload delete --dir `: which project's manifest holds the binding to clear, matching the flag `up` and `config` already take. ## Fixed - `dr artifact code sync` no longer overwrites or deletes a local file without a recoverable copy. Before the remote wins, your version is saved as `.LOCAL.`, for every case that touches the working tree — not only conflicts, as before — including a remote-modified download that used to be applied silently with no backup. Those `.LOCAL` copies are also excluded from the next sync, so a backup is never re-uploaded as new content. +- A deploy that ends errored or terminated now names the way out. It stopped at "check the logs", which is where recovery ran out of documented steps: the remedy people reached for was hand-deleting the `workloadId`, after which the create hit a 409 whose message advised setting the exact line just deleted. It now names `dr workload delete --dir `, which clears the binding with the workload, and `--recreate`, which does both inside the deploy. +- No refusal in `dr workload up` ends at deleting a file or a state directory any more. The remaining one told a project whose linked artifact another workload already owns to delete `.datarobot/workload/`; it now names `dr artifact create` and `dr artifact code init --force`, which keep the code catalog the deletion would have discarded. `dr artifact code sync` against a locked artifact, `dr artifact code init` against a linked directory, and the artifact note printed by `dr workload delete` were each ending the same way and now name the same command. - `dr workload delete` now clears a `workloadId` written as a YAML alias. The reader resolves the alias before answering, so such a binding is the one the project deploys to, but the removal compared the unresolved node and left the file as stale as it found it while reporting the delete a success. - `dr workload up --dir ` now says which flag named a path that is not a directory, instead of failing with a bare `not a directory`, matching what `dr workload delete --dir` already said. - An empty `.datarobot.yaml` now gets one explanation and one remedy whichever command reaches it, rather than a generic "root must be a YAML mapping" from the write path and a friendlier sentence from the read path. @@ -31,6 +35,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - **`dr artifact code sync` no longer lets the remote silently win over your local changes.** When a file changed both locally and on the remote (a conflict), an interactive run asks before pulling, and a non-interactive run (`--yes` or `DATAROBOT_CLI_NON_INTERACTIVE`) is now **refused** unless you pass the new `--accept-remote` flag — so an automated sync fails loudly instead of overwriting your edits. **This is a behavior change for CI:** a pipeline running `dr artifact code sync --yes` that hits a conflict will now exit non-zero where it previously applied remote-wins; add `--accept-remote` to opt back in (your versions are still saved as `*.LOCAL` copies). A plain pull of a remote change you had not touched still applies without prompting. - `dr workload up` no longer pastes the workload id into the commands it suggests running next. They are printed as they can be typed from the project it just deployed, carrying `--dir` when the deploy did. A run that failed after creating the workload names the id above them instead, because that is the one case where the manifest holds no binding for them to resolve. -- `dr workload delete` now removes the `workloadId` it wrote from a `.datarobot.yaml` bound to the workload it deleted, so the next `dr workload up` is not pointed at something that no longer exists. Only a manifest naming that exact id is touched, the id is compared inside the same edit that removes it, and the artifact link under `.datarobot/` is left alone so the next deploy reuses the artifact; the command names that artifact and how to unlink from it. A workload that was already gone leaves the binding alone: a 404 means "not on this instance", which is not proof the binding is stale. +- `dr workload delete` now removes the `workloadId` it wrote from a `.datarobot.yaml` bound to the workload it deleted, so the next `dr workload up` is not pointed at something that no longer exists. Only a manifest naming that exact id is touched, the id is compared inside the same edit that removes it, and the artifact link under `.datarobot/` is left alone so the next deploy reuses the artifact; the command names that artifact and the command that points the project at a different one. A workload that was already gone leaves the binding alone: a 404 means "not on this instance", which is not proof the binding is stale. - `dr workload up` now mints a new artifact when the one this project is linked to no longer says what `.datarobot.yaml` says. An artifact's spec is fixed when it is created, so reusing a diverged one deployed the frozen answer and reported success. A read that fails is not treated as a difference: the run stops and names the artifact it could not read, so a timeout never starts a new artifact lineage. - `dr workload up` now treats a binding that resolves to nothing as drift and creates a new workload, instead of refusing and asking for the id to be cleared by hand. The plan and the JSON envelope both name the id it could not find, so a run pointed at the wrong instance is visible rather than silent. A terminated workload is still refused, because it continues to exist and to hold its name and artifact; the message now names `dr workload delete`, which clears the way and the binding together. diff --git a/cmd/artifact/code/init/cmd.go b/cmd/artifact/code/init/cmd.go index 26b650c1f..62759b7c4 100644 --- a/cmd/artifact/code/init/cmd.go +++ b/cmd/artifact/code/init/cmd.go @@ -74,6 +74,9 @@ Example: c.Flags().String("dir", "", "Project directory (default: current directory).") c.Flags().BoolP(cli.YesFlagName, "y", false, "Skip interactive prompts; use defaults.") + c.Flags().Bool("force", false, + "Re-point an already-linked directory at this artifact instead of refusing. The catalog and last "+ + "synced version are taken from the artifact named here, so the next sync uploads what it lacks.") // Bind only the env var (DATAROBOT_CLI_NON_INTERACTIVE) to viper. The --yes // flag itself is read directly from cmd.Flags() in runInit so an explicit @@ -82,10 +85,12 @@ Example: telemetry.TrackWith(c, func(cmd *cobra.Command, args []string) map[string]any { yes := cli.IsNonInteractive(cmd) + force, _ := cmd.Flags().GetBool("force") return map[string]any{ "artifact_id": telemetry.FirstArg(args), "yes": yes, + "force": force, "output_format": string(outputFormat), } }) @@ -104,7 +109,10 @@ func runInit(cmd *cobra.Command, args []string, outputFormat outputformat.Output format.StateNotice(cmd.ErrOrStderr(), wapi.EnsureMigrated(dir)) - if wapi.Exists(dir) { + force, _ := cmd.Flags().GetBool("force") + + linked := wapi.Exists(dir) + if linked && !force { return reportAlreadyLinked(dir) } @@ -121,6 +129,14 @@ func runInit(cmd *cobra.Command, args []string, outputFormat outputformat.Output codeRef := workload.ExtractCodeRef(*art) opts := buildInitOptions(artifactID, codeRef) + // Re-pointing and linking are one command because they are one question: + // which artifact does this directory push to. Splitting them would leave + // --force to be discovered separately from the thing it fixes, which is how + // "delete the state directory" became the answer people found first. + if linked { + return relink(dir, *art, opts, outputFormat) + } + if err := wapi.Initialize(dir, opts); err != nil { if errors.Is(err, wapi.ErrAlreadyLinked) { return reportAlreadyLinked(dir) @@ -132,6 +148,63 @@ func runInit(cmd *cobra.Command, args []string, outputFormat outputformat.Output return renderInitResult(outputFormat, newInitResult(*art, dir)) } +// relink moves an existing link, naming where it came from. +// +// The previous artifact is read before the write, because after it there is +// nothing left that remembers: a user who re-pointed the wrong directory has +// the id they need to put it back only if this line printed it. +func relink(dir string, art workload.Artifact, opts wapi.InitOptions, outputFormat outputformat.OutputFormat) error { + previous := "" + if cfg, err := wapi.LoadConfig(dir); err == nil { + previous = cfg.ArtifactID + } + + // Re-pointing at the artifact already linked is not a re-point, and doing + // it anyway is pure loss rather than a harmless rewrite. Relink rebuilds + // the sync baseline from the artifact named, which is correct when that is + // a different one and destructive when it is not: with BASE emptied and + // lastSyncedVersionId cleared, the next sync sees a file that exists on + // both sides with different bytes as an ADD_CONFLICT rather than a plain + // local edit, renames it to .LOCAL., and downloads the + // remote copy over it. The same edit uploads cleanly against the baseline + // this call would have discarded. + // + // So the answer being already correct makes this a no-op, not an error: + // --force says do not refuse, and the end state it asks for is the one the + // directory is in. + if previous == opts.ArtifactID { + if outputFormat == outputformat.OutputFormatJSON { + return renderInitResult(outputFormat, newInitResult(art, dir)) + } + + printLinkUnchanged(opts.ArtifactID, dir) + + return nil + } + + if err := wapi.Relink(dir, opts); err != nil { + return err + } + + // The JSON envelope is the shape a fresh link produces plus the one thing + // only a relink has: the artifact it came from. A caller parsing this is + // asking which artifact the directory pushes to, and the id it stopped + // pushing to is the undo — the same id the text path prints, which is why + // it cannot be the text path's alone. + if outputFormat == outputformat.OutputFormatJSON { + result := newInitResult(art, dir) + if previous != "" { + result.PreviousArtifactID = &previous + } + + return renderInitResult(outputFormat, result) + } + + printRelinked(previous, opts.ArtifactID, dir) + + return nil +} + func fetchArtifact(artifactID string) (*workload.Artifact, error) { art, err := getArtifactFn(artifactID) if err != nil { diff --git a/cmd/artifact/code/init/cmd_test.go b/cmd/artifact/code/init/cmd_test.go index e4d85111e..eed233447 100644 --- a/cmd/artifact/code/init/cmd_test.go +++ b/cmd/artifact/code/init/cmd_test.go @@ -21,6 +21,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/datarobot/cli/internal/config/viperx" "github.com/datarobot/cli/internal/drapi" @@ -325,3 +326,208 @@ func TestCmd_DoesNotClobberGlobalYesViper(t *testing.T) { assert.False(t, viperx.GetBool("yes"), "init's --yes must not be bound to global viper key 'yes' (would clobber dotenv)") } + +// --force is the command that "delete the state directory" used to stand in +// for. It re-points the link and leaves everything a deletion would have taken. +func TestRunE_ForceRelinksAnAlreadyLinkedDirectory(t *testing.T) { + tmp := t.TempDir() + + require.NoError(t, wapi.Initialize(tmp, wapi.InitOptions{ + ArtifactID: "68b0aaaa0000000000000001", + CatalogID: "68b0bbbb0000000000000002", + })) + + withFakeArtifact(t, func(id string) (*workload.Artifact, error) { + return fakeArtifact(id, "my-agent", "DRAFT", &workload.DatarobotCodeRef{ + CatalogID: "68b0eeee0000000000000005", + }), nil + }) + + cmd := newTestCmd(t, tmp, true, []string{"68b0dddd0000000000000004"}) + require.NoError(t, cmd.Flags().Set("force", "true")) + + out := captureStdout(t, func() { + require.NoError(t, cmd.Execute()) + }) + + assert.Contains(t, out, "68b0aaaa0000000000000001", "name what it was pointing at, so it can be put back") + assert.Contains(t, out, "68b0dddd0000000000000004") + + cfg, err := wapi.LoadConfig(tmp) + require.NoError(t, err) + assert.Equal(t, "68b0dddd0000000000000004", cfg.ArtifactID) + + require.NotNil(t, cfg.CatalogID) + assert.Equal(t, "68b0eeee0000000000000005", *cfg.CatalogID, + "the catalog comes from the artifact being linked to") +} + +// Without --force the refusal stands, and it has to name the flag rather than +// send the reader to delete the directory. +func TestRunE_WithoutForceStillRefusesAndNamesTheFlag(t *testing.T) { + tmp := t.TempDir() + + require.NoError(t, wapi.Initialize(tmp, wapi.InitOptions{ArtifactID: "68b0aaaa0000000000000001"})) + + withFakeArtifact(t, func(string) (*workload.Artifact, error) { + t.Fatal("a refusal must not cost an API call") + + return nil, nil + }) + + cmd := newTestCmd(t, tmp, true, []string{"68b0dddd0000000000000004"}) + + var err error + + out := captureStdout(t, func() { err = cmd.Execute() }) + + require.Error(t, err) + assert.Contains(t, out, "--force") + assert.NotContains(t, out, "to re-init", "recovery must never end at deleting the state directory") + + cfg, lerr := wapi.LoadConfig(tmp) + require.NoError(t, lerr) + assert.Equal(t, "68b0aaaa0000000000000001", cfg.ArtifactID, "a refusal changes nothing") +} + +// The id being replaced is the undo, and it has to survive the format the +// scripts use. --force returned before printRelinked, so JSON was the one way +// to run this command that lost it. +func TestRunE_ForceJSONCarriesTheArtifactItReplaced(t *testing.T) { + tmp := t.TempDir() + + require.NoError(t, wapi.Initialize(tmp, wapi.InitOptions{ArtifactID: "68b0aaaa0000000000000001"})) + + withFakeArtifact(t, func(id string) (*workload.Artifact, error) { + return fakeArtifact(id, "my-agent", "DRAFT", nil), nil + }) + + cmd := newTestCmd(t, tmp, true, []string{"68b0dddd0000000000000004"}) + require.NoError(t, cmd.Flags().Set("force", "true")) + require.NoError(t, cmd.Flags().Set("output-format", "json")) + + out := captureStdout(t, func() { + require.NoError(t, cmd.Execute()) + }) + + var result map[string]any + + require.NoError(t, json.Unmarshal([]byte(out), &result)) + assert.Equal(t, "68b0dddd0000000000000004", result["artifactId"]) + assert.Equal(t, "68b0aaaa0000000000000001", result["previousArtifactId"], + "the id it stopped pushing to is how a mistaken --force is undone") +} + +// A fresh link replaced nothing, so the field is present and null rather than +// carrying an id from somewhere else. +func TestRunE_JSONHasNoPreviousArtifactForAFreshLink(t *testing.T) { + tmp := t.TempDir() + + withFakeArtifact(t, func(id string) (*workload.Artifact, error) { + return fakeArtifact(id, "my-agent", "DRAFT", nil), nil + }) + + cmd := newTestCmd(t, tmp, true, []string{"68b0dddd0000000000000004"}) + require.NoError(t, cmd.Flags().Set("output-format", "json")) + + out := captureStdout(t, func() { + require.NoError(t, cmd.Execute()) + }) + + var result map[string]any + + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Contains(t, result, "previousArtifactId") + assert.Nil(t, result["previousArtifactId"]) +} + +// --force at the artifact already linked is not a re-point, and performing one +// anyway would be pure loss: Relink rebuilds the sync baseline from the +// artifact named, so BASE and lastSyncedVersionId would be discarded and the +// next sync would see every unsynced local edit as an ADD_CONFLICT — renamed +// to .LOCAL., with the remote copy downloaded over it. +func TestRunE_ForceWithTheArtifactAlreadyLinkedChangesNothing(t *testing.T) { + tmp := t.TempDir() + artifactID := "68b0aaaa0000000000000001" + + require.NoError(t, wapi.Initialize(tmp, wapi.InitOptions{ + ArtifactID: artifactID, + CatalogID: "68b0bbbb0000000000000002", + LastSyncedVersionID: "68b0cccc0000000000000003", + })) + + // A baseline with something in it. An empty one would pass this test + // whether or not the guard exists. + syncedVersion := "68b0cccc0000000000000003" + + syncedAt := time.Date(2026, 8, 1, 12, 0, 0, 0, time.UTC) + + base := wapi.Manifest{ + Version: 1, + SyncedAt: &syncedAt, + SyncedVersionID: &syncedVersion, + Files: map[string]wapi.FileMeta{ + "app.py": {Hash: strings.Repeat("a", 64), Size: 12}, + }, + } + require.NoError(t, wapi.SaveManifest(tmp, base)) + + withFakeArtifact(t, func(id string) (*workload.Artifact, error) { + return fakeArtifact(id, "my-agent", "DRAFT", nil), nil + }) + + cmd := newTestCmd(t, tmp, true, []string{artifactID}) + require.NoError(t, cmd.Flags().Set("force", "true")) + + out := captureStdout(t, func() { + require.NoError(t, cmd.Execute()) + }) + + assert.Contains(t, out, "nothing to change") + + after, err := wapi.LoadManifest(tmp) + require.NoError(t, err) + assert.Equal(t, base.Files, after.Files, "the sync baseline is what a needless relink destroys") + + require.NotNil(t, after.SyncedVersionID) + assert.Equal(t, syncedVersion, *after.SyncedVersionID) + + cfg, err := wapi.LoadConfig(tmp) + require.NoError(t, err) + assert.Equal(t, artifactID, cfg.ArtifactID) + + require.NotNil(t, cfg.LastSyncedVersionID) + assert.Equal(t, syncedVersion, *cfg.LastSyncedVersionID, + "clearing this alone would send the next sync into a full three-way diff") +} + +// The no-op reports the link it left in place, and claims to have replaced +// nothing — because it did not. +func TestRunE_ForceWithTheSameArtifactJSONReportsNoPrevious(t *testing.T) { + tmp := t.TempDir() + artifactID := "68b0aaaa0000000000000001" + + require.NoError(t, wapi.Initialize(tmp, wapi.InitOptions{ArtifactID: artifactID})) + + withFakeArtifact(t, func(id string) (*workload.Artifact, error) { + return fakeArtifact(id, "my-agent", "DRAFT", nil), nil + }) + + cmd := newTestCmd(t, tmp, true, []string{artifactID}) + require.NoError(t, cmd.Flags().Set("force", "true")) + require.NoError(t, cmd.Flags().Set("output-format", "json")) + + out := captureStdout(t, func() { + require.NoError(t, cmd.Execute()) + }) + + var result map[string]any + + require.NoError(t, json.Unmarshal([]byte(out), &result)) + assert.Equal(t, artifactID, result["artifactId"]) + assert.Nil(t, result["previousArtifactId"], "nothing was replaced") +} + +func TestCmd_RegistersForceFlag(t *testing.T) { + assert.NotNil(t, Cmd().Flags().Lookup("force")) +} diff --git a/cmd/artifact/code/init/display.go b/cmd/artifact/code/init/display.go index 2c0b980f2..3b40f0673 100644 --- a/cmd/artifact/code/init/display.go +++ b/cmd/artifact/code/init/display.go @@ -25,9 +25,20 @@ import ( ) type initResult struct { - ArtifactID string `json:"artifactId"` - Name string `json:"name"` - Status string `json:"status"` + ArtifactID string `json:"artifactId"` + Name string `json:"name"` + Status string `json:"status"` + + // PreviousArtifactID is the artifact a --force run re-pointed away from, + // and nil for a fresh link, which had none. + // + // It is here because after the write nothing else remembers it: a caller + // that re-pointed the wrong directory has the id it needs to put the link + // back only if this field carried it out. The text path prints the same + // thing, so leaving it off JSON made --output-format json the one way to + // run the command that loses it. + PreviousArtifactID *string `json:"previousArtifactId"` + CatalogID *string `json:"catalogId"` CatalogVersionID *string `json:"catalogVersionId"` Dir string `json:"dir"` @@ -86,13 +97,49 @@ func printLinkedEmptyArtifact(name, artifactID string) { fmt.Println(tui.DimStyle.Render("Run 'dr artifact code sync' to upload your files.")) } +// printAlreadyLinked refuses, and names the command that changes the link. +// +// It used to end at "delete the state directory", which is filesystem surgery +// standing in for a command: it takes the project's ignore file and its history +// along with the one field being changed, and it is the same advice that made +// deleting state the folk cure for every stuck project. func printAlreadyLinked(artifactID, dir string) { - stateDir := wapi.Dir(dir) - fmt.Println(tui.ErrorStyle.Render( - fmt.Sprintf("Already linked to artifact %s; state exists at %s.", artifactID, stateDir), + fmt.Sprintf("Already linked to artifact %s; state exists at %s.", artifactID, wapi.Dir(dir)), + )) + fmt.Println(tui.DimStyle.Render( + "Pass --force to point this directory at another artifact instead.")) +} + +// printLinkUnchanged reports the link that was already what it was asked to +// be, and says what was deliberately not touched. +// +// The second line is there because "nothing to change" invites the reader to +// re-run with something that does change it. What a relink would have changed +// is the sync baseline, and losing that is how an unsynced local edit ends up +// renamed to a .LOCAL copy with the remote downloaded over it. +func printLinkUnchanged(artifactID, dir string) { + fmt.Println(tui.SuccessStyle.Render( + fmt.Sprintf("%s already points at artifact %s; nothing to change.", dir, artifactID), )) - fmt.Println(tui.DimStyle.Render(fmt.Sprintf("Delete %s to re-init.", stateDir))) + fmt.Println(tui.DimStyle.Render( + "The sync baseline was left alone. Rebuilding it against the same artifact would make every " + + "local edit not yet synced lose to the copy already there.")) +} + +// printRelinked says what moved, and from where. +func printRelinked(previous, artifactID, dir string) { + if previous == "" { + fmt.Println(tui.SuccessStyle.Render( + fmt.Sprintf("Pointed %s at artifact %s.", dir, artifactID))) + + return + } + + fmt.Println(tui.SuccessStyle.Render( + fmt.Sprintf("Re-pointed %s from artifact %s to %s.", dir, previous, artifactID))) + fmt.Println(tui.DimStyle.Render( + "The next sync uploads whatever the new artifact does not already hold.")) } func shortVer(s string) string { diff --git a/cmd/artifact/code/init/display_test.go b/cmd/artifact/code/init/display_test.go index 6c4291013..569e59fd7 100644 --- a/cmd/artifact/code/init/display_test.go +++ b/cmd/artifact/code/init/display_test.go @@ -71,7 +71,31 @@ func TestPrintAlreadyLinked_IncludesPath(t *testing.T) { }) assert.Contains(t, out, "Already linked to artifact art-abc-123; state exists at "+wapi.Dir("/tmp/proj")+".") - assert.Contains(t, out, "Delete "+wapi.Dir("/tmp/proj")+" to re-init.") + assert.Contains(t, out, "--force", "the refusal has to name the command that changes the link") + assert.NotContains(t, out, "Delete "+wapi.Dir("/tmp/proj"), + "recovery must never end at deleting the state directory") +} + +// Re-pointing names both ends, because the id being left is the only way back +// for someone who forced the wrong directory. +func TestPrintRelinked_NamesBothArtifacts(t *testing.T) { + out := captureStdout(t, func() { + printRelinked("art-old-111", "art-new-222", "/tmp/proj") + }) + + assert.Contains(t, out, "art-old-111") + assert.Contains(t, out, "art-new-222") + assert.NotContains(t, out, "Delete ", "the whole point of --force is not deleting anything") +} + +// A link with no readable predecessor still says what it now points at. +func TestPrintRelinked_WithoutAPreviousArtifact(t *testing.T) { + out := captureStdout(t, func() { + printRelinked("", "art-new-222", "/tmp/proj") + }) + + assert.Contains(t, out, "art-new-222") + assert.NotContains(t, out, "from artifact") } func TestRenderInitResult_TextWithCodeRef(t *testing.T) { diff --git a/cmd/workload/del/cmd.go b/cmd/workload/del/cmd.go index f18c7cf79..cd17015e6 100644 --- a/cmd/workload/del/cmd.go +++ b/cmd/workload/del/cmd.go @@ -259,12 +259,14 @@ func clearStaleBinding(w io.Writer, dir, workloadID string) { // refuse instead, and a manifest naming a published image or an artifactId // never consults the link at all. // -// The remedy is the state directory, not `dr artifact delete`. Deleting the -// artifact is refused outright while it is locked, and for an unlocked one it -// leaves the link pointing at something gone, which the next deploy reports as -// a bare 404 naming no fix. Removing the directory is what `up` itself already -// tells the user to do when a locked artifact blocks a deploy, so this says -// the same thing rather than inventing a second answer. +// The remedy is `dr artifact code init --force`, not `dr artifact delete` and +// not the directory. Deleting the artifact is refused outright while it is +// locked, and for an unlocked one it leaves the link pointing at something gone, +// which the next deploy reports as a bare 404 naming no fix. Deleting the state +// directory does work, and is what this used to say, but it takes the code +// catalog and the last-synced version with it for the sake of one field — and a +// delete that ends by recommending a second deletion is how removing state +// became the folk cure for a project that will not deploy. func noteLinkedArtifact(w io.Writer, projectDir string) { if !wapi.Exists(projectDir) { return @@ -277,5 +279,5 @@ func noteLinkedArtifact(w io.Writer, projectDir string) { fmt.Fprintln(w, tui.DimStyle.Render( "This project is still linked to artifact "+cfg.ArtifactID+", which was not deleted with the workload. "+ - "To unlink it, delete "+wapi.Dir(projectDir)+".")) + "To point it at a different one, run 'dr artifact code init --force '.")) } diff --git a/cmd/workload/del/cmd_test.go b/cmd/workload/del/cmd_test.go index 702ba5af7..92340ca7f 100644 --- a/cmd/workload/del/cmd_test.go +++ b/cmd/workload/del/cmd_test.go @@ -204,8 +204,10 @@ func TestClearStaleBinding_NamesTheStillLinkedArtifact(t *testing.T) { assert.Contains(t, out, "68b0aaaa0000000000000001") assert.Contains(t, out, "was not deleted with the workload") - assert.Contains(t, out, wapi.Dir(dir), "the remedy has to name the directory to remove") + assert.Contains(t, out, "dr artifact code init --force", "the remedy has to be a command") assert.NotContains(t, out, "dr artifact delete", "advice that dead-ends is what this ticket is fixing") + assert.NotContains(t, out, wapi.Dir(dir), + "a delete that ends by recommending a second deletion is the loop this ticket is fixing") } // A project that never linked to an artifact has nothing to say about one. diff --git a/cmd/workload/up/cmd.go b/cmd/workload/up/cmd.go index da891c703..81dee0869 100644 --- a/cmd/workload/up/cmd.go +++ b/cmd/workload/up/cmd.go @@ -153,6 +153,7 @@ type flags struct { detach bool lock bool force bool + recreate bool importEnv bool updateEnv bool @@ -221,10 +222,20 @@ plan is built against where it landed. A stopped one is started and then reconciled in the same run, which is one command whether the file asks for a start alone or for a start and a new version. +An errored or terminated workload is the one state a deploy cannot rescue: it +still holds its name, so nothing new can take it. --recreate deletes it and +creates it again under the same name, which is the same recovery +'dr workload delete' performs, folded into the deploy. Deleting cannot be +undone, so it asks first unless --yes is passed, and it refuses any other +state: a workload a deploy can act on is deployed onto, not replaced. Check +'dr workload logs' before reaching for it, because a slow start can report +errored and then recover on its own. + Examples: dr workload up dr workload up --dry-run - dr workload up --yes --output-format json`, + dr workload up --yes --output-format json + dr workload up --recreate`, Args: cobra.NoArgs, PreRunE: auth.EnsureAuthenticatedE, SilenceUsage: true, @@ -249,6 +260,7 @@ Examples: "detach": f.detach, "lock": f.lock, "force_build": f.force, + "recreate": f.recreate, "output_format": string(outputFormat), } }) @@ -268,6 +280,9 @@ func addFlags(cmd *cobra.Command, f *flags, poll *pollflags.Set) { "version. Locking is one-way.") cmd.Flags().BoolVar(&f.force, "force-build", false, "Rebuild the image even when the working tree matches what was last synced.") + cmd.Flags().BoolVar(&f.recreate, "recreate", false, + "Delete the bound workload and create it again under the same name. Only a workload that is errored "+ + "or terminated may be recreated; deleting one cannot be undone.") // The same two flags `dr workload config` takes, because the first run of // this command already reads .env: with no manifest it is the wizard. A @@ -317,11 +332,13 @@ func run(cmd *cobra.Command, f flags, poll pollflags.Set, format outputformat.Ou result, runErr := runFn(up.Options{ Dir: dir, NonInteractive: nonInteractive, + Yes: yes, DryRun: f.dryRun, Detach: f.detach, Lock: f.lock, Confirm: rollConfirm(cmd, yes), ForceBuild: f.force, + Recreate: f.recreate, ImportEnv: f.importEnv, UpdateEnv: f.updateEnv, PollInterval: poll.Interval, diff --git a/cmd/workload/up/cmd_test.go b/cmd/workload/up/cmd_test.go index f6ecaa2ac..2563ea9dd 100644 --- a/cmd/workload/up/cmd_test.go +++ b/cmd/workload/up/cmd_test.go @@ -247,6 +247,22 @@ func TestCmd_PassesTheFlagsThrough(t *testing.T) { _, _, err = runCmd(t, "--lock") require.NoError(t, err) assert.True(t, locking.Lock) + + recreating := stubRun(t, deployed(), nil) + + _, _, err = runCmd(t, "--recreate") + require.NoError(t, err) + assert.True(t, recreating.Recreate) +} + +// --recreate is off unless it is asked for: it deletes a workload, and a flag +// that defaulted on would do that to a run that never mentioned it. +func TestCmd_RecreateIsOffByDefault(t *testing.T) { + seen := stubRun(t, deployed(), nil) + + _, _, err := runCmd(t) + require.NoError(t, err) + assert.False(t, seen.Recreate) } // Locking happens after the workload is serving and --detach returns before diff --git a/docs/commands/artifact.md b/docs/commands/artifact.md index 396e9d500..bd29d3495 100644 --- a/docs/commands/artifact.md +++ b/docs/commands/artifact.md @@ -167,13 +167,14 @@ dr artifact build logs [] [--level debug|info|warn|err Synchronize a local project directory with an artifact's source code. Run `init` once to link a directory, then `sync` to push and pull changes. ```bash -dr artifact code init [] [--dir ] [--yes] +dr artifact code init [] [--dir ] [--yes] [--force] dr artifact code sync [--dir ] [--dry-run | --diff] [--yes] dr artifact code versions [--dir ] [--limit N] dr artifact code checkout [] [--dir ] [--clean] ``` - `init` creates the `.datarobot/workload/` state directory and binds it to an existing draft artifact. The artifact must already exist (`dr artifact create` or the DataRobot UI); these commands manage an artifact's code, not its lifecycle. It also drops a starter `.drignore` at the project root, in gitignore syntax, listing what `sync` should leave out. Edit it and commit it. A project that already has an ignore file under either name keeps it, and no new one is written. +- `init --force` re-points a directory that is already linked at a different artifact, instead of refusing. The catalog and last-synced version are taken from the artifact named on the command line, exactly as a first `init` takes them, so the next `sync` uploads whatever that artifact does not already hold. Your `.drignore` and the project's sync history are kept — which is what makes this preferable to deleting `.datarobot/workload/`, the only way to change a link before this flag existed and one that discards the code catalog along with it. The id being replaced is printed, so a directory forced by mistake can be put back; under `--output-format json` it is the `previousArtifactId` field, which is `null` for a first link. Naming the artifact the directory is already linked to does nothing and says so: re-pointing rebuilds the sync baseline from the artifact given, which is what makes a real re-point correct and would, against the same artifact, turn every local edit not yet synced into a conflict resolved in the remote copy's favour. - Projects created before the file was renamed have a `.wapiignore` instead. It is still read when there is no `.drignore` beside it, and `sync` says so once per run, but the name is deprecated: rename the file when convenient. If both are present, `.drignore` is the one that applies and `sync` warns that the other file's patterns are not in effect. Merge them and delete the old one: two ignore files at a project root is a state where patterns you wrote silently stop filtering. The ignore file is uploaded with your code, so if you work with others, agree on the rename rather than doing it alone. - `sync` computes a three-way diff against the last synced state and applies it in one versioned step. Conflicts resolve to the remote copy, and your version is kept as a `*.LOCAL.` file. Preview with `--dry-run`, or use `--diff` to also see per-file diffs. Both exit before any remote write. - For Python projects, the image build requires a `uv.lock` next to `pyproject.toml`. When your project has `pyproject.toml` but no `uv.lock`, `sync` generates one automatically by running your local `uv lock` (your uv configuration, private indexes, and credentials apply) and uploads it with the rest of your code — commit the generated file to your repo. If `uv` is not installed or lock generation fails, sync still completes and prints what to do (`uv lock`, then re-sync); the image build will fail until a lock file is added. This also happens on `--dry-run`/`--diff`, so the preview matches what a real sync would upload. An existing `uv.lock` is never modified, and sync warns if your `.drignore` excludes it. diff --git a/docs/commands/workload.md b/docs/commands/workload.md index 3b69f7789..ab533c765 100644 --- a/docs/commands/workload.md +++ b/docs/commands/workload.md @@ -154,7 +154,7 @@ dr workload delete [] [--dir ] [--yes] - `--yes`, `-y`: skip the confirmation prompt. `DATAROBOT_CLI_NON_INTERACTIVE=1` stands in for it only when you passed the workload id; a workload whose id is specified in the manifest takes the explicit flag. - `--dir `: project directory whose `.datarobot.yaml` names the workload, and holds the binding to clear, searched upward from there. Defaults to the current directory. Pass the same value you deployed with, since a manifest in a subdirectory is not visible from its parent. -If the manifest found from `--dir` is bound to the workload just deleted, the `workloadId` line the CLI wrote is removed with it, so the next `dr workload up` creates a new workload instead of pointing at one that is gone. Only a manifest naming that exact id is touched. The artifact link under `.datarobot/` is left alone, because the artifact itself survives the deletion. The command names the artifact so the link is not left invisible, and names the state directory to remove if you want to unlink from it. These notes go to stderr, so stdout stays the command's result. +If the manifest found from `--dir` is bound to the workload just deleted, the `workloadId` line the CLI wrote is removed with it, so the next `dr workload up` creates a new workload instead of pointing at one that is gone. Only a manifest naming that exact id is touched. The artifact link under `.datarobot/` is left alone, because the artifact itself survives the deletion. The command names the artifact so the link is not left invisible, and names `dr artifact code init --force ` as the way to point the project at a different one. These notes go to stderr, so stdout stays the command's result. A manifest this edit cannot make sound again is refused rather than rewritten, with the reason and the remedy: a binding whose value something else aliases, repeated `workloadId` keys that disagree, a file whose only recognized key is the binding, and a file carrying more than one YAML document. @@ -262,6 +262,26 @@ dr workload start dr workload delete ``` +## Recovering a stuck workload + +An errored or terminated workload is the one state a deploy cannot rescue. It still holds its name, so the next `dr workload up` can neither deploy onto it nor create a replacement beside it. Recovery is to remove it and deploy again, and the CLI names that exit wherever it refuses: + +```bash +# Diagnose first: a slow start can report errored and then recover on its own. +dr workload logs + +# Either remove it explicitly, which also clears the binding in .datarobot.yaml, +dr workload delete --dir . +dr workload up + +# or fold both into the deploy. +dr workload up --recreate +``` + +`--recreate` deletes the bound workload and creates it again under the same name. It acts only on a workload that is errored or terminated; any other state is refused, because a workload a deploy can act on is deployed onto rather than replaced. Deleting cannot be undone, so it asks for the workload name to be typed back unless `--yes` is passed, and `--dry-run --recreate` deletes nothing while still planning against what would follow the delete, so the plan it prints is the one the real run carries out. + +Do not remove the `workloadId` line by hand, and do not delete `.datarobot/`. Clearing the binding alone leaves the old workload holding the name, so the create that follows fails; deleting the state directory discards the code catalog and the last-synced version, so the next sync re-uploads a tree the platform already has. + ## Error handling | Status | Cause | diff --git a/internal/workload/sync/phase1_gather.go b/internal/workload/sync/phase1_gather.go index cdee6693f..20a959b9b 100644 --- a/internal/workload/sync/phase1_gather.go +++ b/internal/workload/sync/phase1_gather.go @@ -62,9 +62,9 @@ func phase1Gather(e *Engine) error { "artifact %s is locked (immutable); cannot sync.\n"+ " A project with a %s deploys past this on its own: 'dr workload up' creates a new "+ "version, points this directory at it, and rolls onto it.\n"+ - " Otherwise make one with 'dr artifact create' (or in the DataRobot UI), then delete %s "+ - "and re-link with 'dr artifact code init '", - art.ID, wlmanifest.FileName, wapi.Dir(e.projectDir)) + " Otherwise make one with 'dr artifact create' (or in the DataRobot UI), then point this "+ + "directory at it with 'dr artifact code init --force '", + art.ID, wlmanifest.FileName) } e.lockedNote = fmt.Sprintf( diff --git a/internal/workload/sync/phase1_locked_test.go b/internal/workload/sync/phase1_locked_test.go new file mode 100644 index 000000000..b625c2055 --- /dev/null +++ b/internal/workload/sync/phase1_locked_test.go @@ -0,0 +1,67 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sync + +import ( + "testing" + + "github.com/datarobot/cli/internal/workload" + "github.com/datarobot/cli/internal/workload/wapi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// lockedArtifacts is the one call phase 1 makes, answering with an artifact +// nothing can be pushed into. +type lockedArtifacts struct{} + +func (lockedArtifacts) Get(id string) (*workload.Artifact, error) { + return &workload.Artifact{ID: id, Status: workload.ArtifactStatusLocked}, nil +} + +func (lockedArtifacts) PatchCodeRef(_, _, _ string) error { return nil } + +// A locked artifact refuses the sync, and the refusal has to name a command. +// It used to end at deleting the state directory, which takes the code catalog +// and the last-synced version along with the one field being changed. +func TestPhase1Gather_LockedArtifactNamesACommandNotADirectory(t *testing.T) { + dir := t.TempDir() + require.NoError(t, wapi.Initialize(dir, wapi.InitOptions{ArtifactID: "68b0aaaa0000000000000001"})) + + e, err := newWithDeps(dir, Options{}, Deps{Artifacts: lockedArtifacts{}}) + require.NoError(t, err) + + err = phase1Gather(e) + require.Error(t, err) + + assert.Contains(t, err.Error(), "is locked (immutable)") + assert.Contains(t, err.Error(), "dr artifact code init --force", + "the way out has to be a command the reader can run") + assert.NotContains(t, err.Error(), "delete "+wapi.Dir(dir), + "recovery must never end at deleting the state directory") +} + +// A preview is not a write, so it reports the lock rather than refusing: the +// deploy path asks for this diff before deciding to mint a new version. +func TestPhase1Gather_LockedArtifactStillPreviews(t *testing.T) { + dir := t.TempDir() + require.NoError(t, wapi.Initialize(dir, wapi.InitOptions{ArtifactID: "68b0aaaa0000000000000001"})) + + e, err := newWithDeps(dir, Options{DryRun: true}, Deps{Artifacts: lockedArtifacts{}}) + require.NoError(t, err) + + require.NoError(t, phase1Gather(e)) + assert.Contains(t, e.lockedNote, "preview only") +} diff --git a/internal/workload/up/build.go b/internal/workload/up/build.go index 20dd1fcbd..716bc870f 100644 --- a/internal/workload/up/build.go +++ b/internal/workload/up/build.go @@ -105,6 +105,12 @@ func buildAndCreate(loaded Loaded, code CodeChange, result Result, opts Options, // up new ones. When it goes stale, the platform's own message names an // artifact id and nothing about where that id came from, which leaves the // reader with a conflict and no idea what chose the thing that conflicted. +// +// The way out is a command rather than the directory. Telling the reader to +// delete the link was accurate and still wrong: it throws away the code catalog +// and the last-synced version along with the one field being changed, so the +// next sync re-uploads a tree the platform already holds, and it taught deleting +// state as the cure for a project that will not deploy. func reusedArtifactConflict(createErr error, artifactID, projectDir string) error { if !isConflict(createErr) { return createErr @@ -125,10 +131,11 @@ func reusedArtifactConflict(createErr error, artifactID, projectDir string) erro " The link lives in %s and is what makes repeated deploys update one artifact rather than "+ "making a new one each time.\n"+ " To deploy to that workload, set 'workloadId: %s' in %s, replacing any already there.\n"+ - " To deploy a separate workload from this directory, delete %s: the next run then creates "+ - "an artifact of its own.\n"+ + " To deploy a separate workload from this directory, make an artifact of its own with "+ + "'dr artifact create' and point this project at it with "+ + "'dr artifact code init --force '.\n"+ " %w", - artifactID, owner, wapi.Dir(projectDir), owner, manifest.FileName, wapi.Dir(projectDir), createErr) + artifactID, owner, wapi.Dir(projectDir), owner, manifest.FileName, createErr) } // workloadOn names the workload already using artifactID, "" when the lookup diff --git a/internal/workload/up/recreate.go b/internal/workload/up/recreate.go new file mode 100644 index 000000000..815b54b8e --- /dev/null +++ b/internal/workload/up/recreate.go @@ -0,0 +1,161 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package up + +import ( + "errors" + "fmt" + + "github.com/datarobot/cli/internal/workload/manifest" + "github.com/datarobot/cli/tui" +) + +// recreate performs the exit every dead-workload refusal names: delete what is +// holding the name, clear the binding, and let the run create it again. +// +// It exists because naming a remedy is not the same as being able to reach it. +// The refusals spell out 'dr workload delete --dir ', which works, +// but it is a second command against a state the user did not choose and cannot +// deploy out of; --recreate is that command folded into the deploy that was +// already asked for. +// +// The live state it returns is what the plan is built from, which is why this +// runs before the plan rather than beside the apply. Once the workload is gone +// the run is a create, and a plan that still described a roll would be +// describing something that no longer exists. +// +// It widens to nothing else. Deleting is irreversible, so every state a deploy +// can actually act on is refused rather than quietly folded in: --recreate on a +// running workload would otherwise mean "destroy production and rebuild it", +// which is not what the flag says and not what anyone reaching for it wants. +func recreate(loaded Loaded, live Live, opts Options) (Live, error) { + if !opts.Recreate { + return live, nil + } + + // Nothing to delete. A first run, or one whose binding resolved to nothing, + // already ends in a create, so the flag is satisfied by doing nothing rather + // than by failing a run that was about to do exactly what was asked. + if live.State == StateUnbound || live.State == StateMissing { + return live, nil + } + + if live.State != StateErrored && live.State != StateTerminated { + return live, fmt.Errorf( + "--recreate deletes and rebuilds a workload that is errored or terminated, and workload %s is %s. "+ + "Deploy without it: a workload in that state is deployed onto rather than replaced", + name(loaded, live), live.Status) + } + + // A dry run has to stay one, and has to stay honest. This is the only + // mutation that happens before the plan is printed, so the delete is + // skipped — but the state handed back is still the one the real run would + // reach, because the plan printed here is the whole answer to "what would + // this deploy". + // + // Returning the dead workload instead described a deploy that cannot + // happen: the plan rendered a roll onto something the next line refuses, + // and the envelope reported action "rolled" for a run whose real action is + // "created". Nothing mutates on the strength of this, because Run stops at + // its own DryRun branch before anything is applied. + if opts.DryRun { + fmt.Fprintln(opts.Stderr, tui.HintStyle.Render(fmt.Sprintf( + "--recreate would delete workload %s (%s) and create it again under the same name. "+ + "The plan below is the one that would run after it.", + live.WorkloadID, live.Status))) + + return Live{State: StateUnbound}, nil + } + + agreed, err := confirmRecreate(loaded, live, opts) + if err != nil || !agreed { + return live, err + } + + report := newReporter(opts.Stderr, opts.Spinner) + + if err := report.run("Deleting the "+live.Status+" workload", func() error { + return deleteWorkloadFn(live.WorkloadID) + }); err != nil { + return live, fmt.Errorf("cannot delete workload %s, so it still holds its name: %w", live.WorkloadID, err) + } + + clearBinding(loaded, live, report) + + // Unbound rather than missing: the binding is gone from the file, not merely + // pointing at something that answers 404, and the render each state produces + // is the difference between "will be created" and "is bound to an id that no + // longer exists". + return Live{State: StateUnbound}, nil +} + +// clearBinding takes the workloadId out of the manifest, and reports rather +// than fails when it cannot. +// +// The delete has already happened by this point, so failing here would stop a +// run whose irreversible half is done, over a line the create is about to +// overwrite anyway: WriteWorkloadID replaces the key in place. A clear that +// fails and a create that then fails leaves the file bound to a deleted +// workload, which the next run reads as drift and recreates — the state this +// flag exists to get out of, not a new one. +func clearBinding(loaded Loaded, live Live, report *reporter) { + cleared, err := clearWorkloadIDFn(loaded.Path, live.WorkloadID) + + switch { + case err != nil: + report.say(" Could not remove %s from %s: %v\n", "workloadId", loaded.Path, err) + case cleared: + report.say(" Removed the binding to %s from %s.\n", live.WorkloadID, manifest.FileName) + } +} + +// confirmRecreate asks before the one irreversible thing this flag does. +// +// Typing the name back is the gesture confirmLock already uses for rolling a +// locked production version, and for the same reason: a y/n on a destructive +// question is answered by reflex, and the workload being deleted is the one +// piece of information that makes the answer considered. +// +// The three-way shape mirrors confirmRoll's. Being able to ask settles it; +// --yes is a caller who has already said so; and no terminal with no --yes is +// the case that must not proceed silently. +// +// Where it deliberately parts from confirmRoll is the middle branch, which +// reads Yes rather than NonInteractive. NonInteractive is also true for a run +// whose stdin is a pipe, so keying on it here meant +// `dr workload up --recreate < /dev/null` deleted the workload with no prompt +// and no --yes, and made the refusal below unreachable from the command. +// `dr workload delete` settles the same question the same way, and the flag +// help, the docs and the changelog all promise this one asks unless --yes. +// Rolling a locked version can be waved through by a reviewed pipeline; +// deleting cannot, because there is nothing on the other side of it. +func confirmRecreate(loaded Loaded, live Live, opts Options) (bool, error) { + workloadName := name(loaded, live) + + if opts.Confirm != nil { + return opts.Confirm(fmt.Sprintf( + "Workload %s is %s. It will be deleted, which cannot be undone, and created again "+ + "under the same name.\nType the workload name to recreate it, anything else to stop: ", + tui.WarnStyle.Render("`"+workloadName+"`"), live.Status), workloadName) + } + + if opts.Yes { + return true, nil + } + + return false, errors.New( + "--recreate deletes a workload, which cannot be undone, and there is no terminal to confirm on. " + + "Re-run with --yes to say so explicitly") +} diff --git a/internal/workload/up/recreate_test.go b/internal/workload/up/recreate_test.go new file mode 100644 index 000000000..6c11bbba3 --- /dev/null +++ b/internal/workload/up/recreate_test.go @@ -0,0 +1,303 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package up + +import ( + "errors" + "testing" + "time" + + "github.com/datarobot/cli/internal/workload" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// boundDeadManifest is a project bound to a workload that is past saving, +// which is the only state --recreate acts on. +const boundWorkloadID = "68b0c1d2e3f4a5b6c7d8e9f0" + +func deadWorkload(status string) func(string) (workload.Document, error) { + return func(string) (workload.Document, error) { + return workload.Document{"id": boundWorkloadID, "name": "my-app", "status": status}, nil + } +} + +func boundManifestFor() string { + return "workloadId: " + boundWorkloadID + "\n" + unboundImageManifest +} + +// The whole point of the flag: the run that used to refuse now performs the +// exit its refusal names, and comes out the far side having created the +// workload again under the same name. +func TestRecreate_DeletesTheDeadWorkloadAndCreatesItAgain(t *testing.T) { + for _, status := range []string{workload.WorkloadStatusErrored, workload.WorkloadStatusTerminated} { + t.Run(status, func(t *testing.T) { + var ( + deleted string + clearedID string + ) + + install(t, fakes{ + workloadD: deadWorkload(status), + artifactD: func(string) (workload.Document, error) { return nil, nil }, + deleteWorkload: func(id string) error { + deleted = id + + return nil + }, + clearID: func(_, id string) (bool, error) { + clearedID = id + + return true, nil + }, + create: func(any) (*workload.Workload, error) { return running("wl-new"), nil }, + wait: func(string, workload.Serving, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + return running("wl-new"), nil + }, + }) + + result, stderr, err := runIn(t, boundManifestFor(), + Options{NonInteractive: true, Yes: true, Recreate: true}) + require.NoError(t, err) + + assert.Equal(t, boundWorkloadID, deleted, "the dead workload is what gets deleted") + assert.Equal(t, boundWorkloadID, clearedID, "and the binding to it goes with it") + assert.Equal(t, "wl-new", result.WorkloadID, "the run continues into the create") + assert.Equal(t, "my-app", result.Name, "recreated under the name the file gives") + assert.Contains(t, stderr, "Deleting the "+status+" workload") + }) + } +} + +// --recreate is not a way to replace a workload a deploy can act on. Widening +// it to a running one would make it "destroy production and rebuild it", which +// is not what it says. +func TestRecreate_RefusesAWorkloadADeployCanActOn(t *testing.T) { + install(t, fakes{ + workloadD: func(string) (workload.Document, error) { return doc(t, liveWorkloadJSON), nil }, + artifactD: func(string) (workload.Document, error) { return nil, nil }, + }) + + _, _, err := runIn(t, boundManifestFor(), Options{NonInteractive: true, Yes: true, Recreate: true}) + require.Error(t, err) + + assert.Contains(t, err.Error(), "--recreate") + assert.Contains(t, err.Error(), "errored or terminated", "say what it does act on") +} + +// A first run has nothing to delete, and failing it would refuse a run that was +// already about to do exactly what was asked. +func TestRecreate_NoOpsWhenThereIsNothingBound(t *testing.T) { + install(t, fakes{ + create: func(any) (*workload.Workload, error) { return running("wl-new"), nil }, + wait: func(string, workload.Serving, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + return running("wl-new"), nil + }, + }) + + result, _, err := runIn(t, unboundImageManifest, Options{NonInteractive: true, Yes: true, Recreate: true}) + require.NoError(t, err) + assert.Equal(t, "wl-new", result.WorkloadID) +} + +// The one ordering hazard: the delete happens before the plan is printed, so a +// dry run that deleted would be a dry run that mutated. The unwired delete seam +// fails the test if this regresses. +func TestRecreate_DryRunDeletesNothing(t *testing.T) { + install(t, fakes{ + workloadD: deadWorkload(workload.WorkloadStatusErrored), + artifactD: func(string) (workload.Document, error) { return nil, nil }, + }) + + _, stderr, err := runIn(t, boundManifestFor(), + Options{NonInteractive: true, Yes: true, Recreate: true, DryRun: true}) + require.NoError(t, err) + + assert.Contains(t, stderr, "would delete workload "+boundWorkloadID) +} + +// A dry run has to answer "what would this deploy", so it has to plan against +// the state the real run reaches rather than the one it starts from. +// +// Skipping the delete but keeping the dead workload described a deploy that +// cannot happen: the plan rendered a roll onto a workload the very next check +// refuses, and the envelope reported "rolled" for a run whose real action is +// "created" — while the hint above it said a create would happen. +func TestRecreate_DryRunPlansTheRunItWouldPerform(t *testing.T) { + install(t, fakes{ + workloadD: deadWorkload(workload.WorkloadStatusErrored), + artifactD: func(string) (workload.Document, error) { return nil, nil }, + }) + + previewed, stderr, err := runIn(t, boundManifestFor(), + Options{NonInteractive: true, Yes: true, Recreate: true, DryRun: true}) + require.NoError(t, err) + + assert.Equal(t, ActionCreated, previewed.Plan.Action(), + "the dead workload is deleted, so what follows is a create") + assert.Contains(t, stderr, "will be created") + assert.NotContains(t, stderr, "nothing healthy to deploy onto", + "the plan must not describe a deploy onto the workload it just said would be deleted") + + // The same run for real, which is the thing the preview claims to predict. + install(t, fakes{ + workloadD: deadWorkload(workload.WorkloadStatusErrored), + artifactD: func(string) (workload.Document, error) { return nil, nil }, + deleteWorkload: func(string) error { return nil }, + clearID: func(string, string) (bool, error) { return true, nil }, + create: func(any) (*workload.Workload, error) { return running("wl-new"), nil }, + wait: func(string, workload.Serving, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + return running("wl-new"), nil + }, + }) + + performed, _, err := runIn(t, boundManifestFor(), Options{NonInteractive: true, Yes: true, Recreate: true}) + require.NoError(t, err) + + assert.Equal(t, performed.Action, previewed.Plan.Action(), + "a preview that names a different action than the run is not a preview") +} + +// Deleting is irreversible, so a run with nobody to ask and no --yes must stop +// rather than assume consent. +// +// The piped case is the one the command actually produces: `up` passes no +// Confirm when stdin is not a terminal and sets NonInteractive at the same +// time, so keying consent on NonInteractive made `--recreate < /dev/null` +// delete without a word and put this refusal out of reach. The unwired delete +// seam is what fails the test if that comes back. +func TestRecreate_WithoutATerminalOrYesRefuses(t *testing.T) { + for name, opts := range map[string]Options{ + "no terminal": {Recreate: true}, + "stdin is a pipe": {Recreate: true, NonInteractive: true}, + } { + t.Run(name, func(t *testing.T) { + install(t, fakes{ + workloadD: deadWorkload(workload.WorkloadStatusErrored), + artifactD: func(string) (workload.Document, error) { return nil, nil }, + }) + + _, _, err := runIn(t, boundManifestFor(), opts) + require.Error(t, err) + + assert.Contains(t, err.Error(), "--yes", "name the way to say so explicitly") + assert.Contains(t, err.Error(), "cannot be undone") + }) + } +} + +// The safety story of the flag is that the workload's name has to be typed +// back, and a prompt that accepts anything is not that. What the deploy hands +// Confirm as the expected answer is the assertion: nothing else in the run +// checks it, so a literal there would go unnoticed. +func TestRecreate_AsksForTheWorkloadNameToBeTypedBack(t *testing.T) { + var ( + asked string + expect string + ) + + install(t, fakes{ + workloadD: deadWorkload(workload.WorkloadStatusErrored), + artifactD: func(string) (workload.Document, error) { return nil, nil }, + deleteWorkload: func(string) error { return nil }, + clearID: func(string, string) (bool, error) { return true, nil }, + create: func(any) (*workload.Workload, error) { return running("wl-new"), nil }, + wait: func(string, workload.Serving, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + return running("wl-new"), nil + }, + }) + + result, _, err := runIn(t, boundManifestFor(), Options{ + Recreate: true, + Confirm: func(question, want string) (bool, error) { + asked, expect = question, want + + return true, nil + }, + }) + require.NoError(t, err) + + assert.Equal(t, "my-app", expect, "the workload name is what has to be typed") + assert.Contains(t, asked, "my-app", "the question names the workload being deleted") + assert.Contains(t, asked, "cannot be undone") + assert.Equal(t, "wl-new", result.WorkloadID, "agreeing carries the run into the create") +} + +// Declining leaves the workload alone and the run does not proceed to create a +// second one beside it. +func TestRecreate_DecliningStopsTheRun(t *testing.T) { + install(t, fakes{ + workloadD: deadWorkload(workload.WorkloadStatusErrored), + artifactD: func(string) (workload.Document, error) { return nil, nil }, + }) + + _, _, err := runIn(t, boundManifestFor(), Options{ + Recreate: true, + Confirm: func(string, string) (bool, error) { return false, nil }, + }) + + // The run stops at the refusal the dead state already carries, having + // deleted nothing: the unwired delete seam would have failed the test. + require.Error(t, err) + assert.Contains(t, err.Error(), "is errored") +} + +// The delete has already happened by the time the binding is cleared, so a +// failure there must not stop a run whose irreversible half is done. +func TestRecreate_AFailedBindingClearDoesNotStopTheRun(t *testing.T) { + install(t, fakes{ + workloadD: deadWorkload(workload.WorkloadStatusErrored), + artifactD: func(string) (workload.Document, error) { return nil, nil }, + deleteWorkload: func(string) error { return nil }, + clearID: func(string, string) (bool, error) { + return false, errors.New("permission denied") + }, + create: func(any) (*workload.Workload, error) { return running("wl-new"), nil }, + wait: func(string, workload.Serving, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + return running("wl-new"), nil + }, + }) + + result, stderr, err := runIn(t, boundManifestFor(), Options{NonInteractive: true, Yes: true, Recreate: true}) + require.NoError(t, err) + + assert.Equal(t, "wl-new", result.WorkloadID) + assert.Contains(t, stderr, "permission denied", "say what could not be written") +} + +// A delete that fails leaves the name still taken, so the run must stop rather +// than walk into the 409 it would cause. +func TestRecreate_AFailedDeleteStopsTheRun(t *testing.T) { + install(t, fakes{ + workloadD: deadWorkload(workload.WorkloadStatusErrored), + artifactD: func(string) (workload.Document, error) { return nil, nil }, + deleteWorkload: func(string) error { return errors.New("boom") }, + }) + + _, _, err := runIn(t, boundManifestFor(), Options{NonInteractive: true, Yes: true, Recreate: true}) + require.Error(t, err) + + assert.Contains(t, err.Error(), "still holds its name") +} diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index b9c708b5f..20d579d43 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -66,6 +66,8 @@ var ( saveProjectFn = wapi.SaveConfig patchCodeRefFn = workload.PatchArtifactCodeRef syncProjectFn = defaultSync + deleteWorkloadFn = workload.DeleteWorkload + clearWorkloadIDFn = manifest.ClearWorkloadID ) // Options is everything a run needs from its caller. @@ -76,8 +78,19 @@ type Options struct { // NonInteractive forbids prompting. With no manifest it turns the setup // wizard into an error naming the command that writes one. + // + // It is not consent. It is true for --output json and for a run with no + // terminal, neither of which is anyone saying yes, so a step that must be + // agreed to reads Yes instead. NonInteractive bool + // Yes is the caller saying so explicitly: --yes, or the env var that + // stands in for it. It is the only thing that stands in for an answer the + // user would otherwise type, which is why it is separate from + // NonInteractive — a piped stdin means nobody can be asked, not that the + // answer is yes. + Yes bool + // DryRun stops after the plan. DryRun bool @@ -132,6 +145,27 @@ type Options struct { // Spinner is true only when Stderr is the terminal the user is watching. Spinner bool + + // Recreate deletes the bound workload and creates it again under the same + // name, for the one case the dead-state refusals have no other answer to: + // an errored or terminated workload still holding its name. It is the + // exit those refusals name, performed rather than described. + // + // It acts on nothing else. A workload a deploy can act on is deployed + // onto, not deleted and rebuilt, so every other state refuses rather than + // silently widening what the flag means. + Recreate bool + + // dirFlag is the " --dir " a remedy printed deep in a run has to + // carry to reach this project. Filled in by Run once the manifest is + // located, and unexported because it is derived from the load rather than + // chosen by the caller. + // + // Carried on Options because the waits that need it are several calls below + // the last function holding a Loaded, and threading a directory through + // settle and awaitRunning for one message would widen four signatures that + // have nothing else to do with the manifest. + dirFlag string } // Result is what happened, and the material for the JSON envelope. @@ -193,7 +227,12 @@ func Run(opts Options) (Result, error) { // while it goes on serving the old value. early := Result{WorkloadID: loaded.WorkloadID(), Env: loaded.Env} - live, err := lookSettled(loaded.WorkloadID(), opts) + // Filled in once, here, because every remedy printed below this line has to + // name a command that reaches this project rather than the one the shell + // happens to be standing in. + opts.dirFlag = dirFlagFor(loaded) + + live, err := lookLive(loaded, opts) if err != nil { return early, err } @@ -688,6 +727,26 @@ func boundID(live Live) string { return live.WorkloadID } +// lookLive is what the plan is built against: the workload as it stands, after +// the run has been given its one chance to clear away a workload that cannot be +// deployed onto at all. +// +// The two belong together because the answer to "what is live" is not settled +// until --recreate has had its say. Asking them separately in Run put a deleted +// workload and the plan describing it one statement apart, which is close enough +// for a later edit to slip between. +func lookLive(loaded Loaded, opts Options) (Live, error) { + live, err := lookSettled(loaded.WorkloadID(), opts) + if err != nil { + return live, err + } + + // Before the plan, so the plan is built and rendered against what the run + // will actually find: once the dead workload is gone this is a create, and + // a plan describing a roll onto something already deleted would be fiction. + return recreate(loaded, live, opts) +} + // dirFlagFor is the " --dir " a remedy has to carry to reach this // project, and empty when the working directory already does. // @@ -942,7 +1001,7 @@ func deployable(live Live, workloadName, dirFlag string) error { case StateTerminated: return fmt.Errorf( "workload %s is terminated, which cannot be undone, and it still holds its name and artifact — "+ - "%s, then deploy again. %s", + "%s, then deploy again, or re-run with --recreate to do both in one step. %s", workloadName, deleteRemedy(live.WorkloadID, dirFlag, true), recreateNote) case StateErrored: @@ -952,7 +1011,8 @@ func deployable(live Live, workloadName, dirFlag string) error { // name conflict below or throws away the code catalog with it. return fmt.Errorf( "workload %s is errored, so there is nothing safe to deploy onto. "+ - "%sIf it stays errored, %s, then deploy again. %s", + "%sIf it stays errored, %s, then deploy again, or re-run with --recreate to do both "+ + "in one step. %s", workloadName, erroredCaveat(live.WorkloadID), deleteRemedy(live.WorkloadID, dirFlag, true), recreateNote) @@ -1321,9 +1381,26 @@ func awaitRunning( return result, err } + // The refusal carries its own exit, the same way deployable's does. This is + // the message that gets there first: deployable speaks for the run after + // the workload is already stuck, while this one is printed at the moment it + // becomes stuck. Ending it at "check the logs" is what left recovery to + // folklore, and the folk remedy — hand-deleting the workloadId, or the whole + // state directory — either loops through the name conflict or throws away + // the code catalog with it. if workload.IsWorkloadErrorStatus(result.Status) { - return result, fmt.Errorf("workload %s finished as %s; check 'dr workload logs %s'", - workloadID, result.Status, workloadID) + // Errored is the one dead-looking state that can still recover, so it is + // hedged with diagnosis first; terminated really is final. + caveat := "" + if !strings.EqualFold(result.Status, workload.WorkloadStatusTerminated) { + caveat = erroredCaveat(workloadID) + } + + return result, fmt.Errorf( + "workload %s finished as %s, so it never came up. %sIf it stays %s, %s, then deploy again, "+ + "or re-run with --recreate to do both in one step. %s", + workloadID, result.Status, caveat, result.Status, + deleteRemedy(workloadID, opts.dirFlag, true), recreateNote) } return result, nil diff --git a/internal/workload/up/run_test.go b/internal/workload/up/run_test.go index 87fc65aa1..2bd4bf77c 100644 --- a/internal/workload/up/run_test.go +++ b/internal/workload/up/run_test.go @@ -214,6 +214,10 @@ type fakes struct { // settings is the in-place path: a change that moved only the sizing. settings func(string, json.RawMessage) (*workload.Replacement, error) + + // The --recreate track: delete what is holding the name, then unbind. + deleteWorkload func(string) error + clearID func(string, string) (bool, error) } // install swaps in the seams the test supplied and restores them afterwards. @@ -268,6 +272,19 @@ func install(t *testing.T, f fakes) { swap(t, &writeWorkloadIDFn, f.writeID) swap(t, &codeChangeFn, f.code) swap(t, &getWorkloadDocFn, f.workloadD) + + // A delete is irreversible and this one is a real DELETE against whatever + // tenant the developer is logged into, so an unwired seam has to fail the + // test rather than reach the network. + force(t, &deleteWorkloadFn, func(id string) error { + t.Fatalf("the run deleted workload %s, which this test did not wire", id) + + return nil + }) + swap(t, &deleteWorkloadFn, f.deleteWorkload) + + force(t, &clearWorkloadIDFn, func(string, string) (bool, error) { return false, nil }) + swap(t, &clearWorkloadIDFn, f.clearID) swap(t, &getArtifactDocFn, f.artifactD) // A test that does not wire the build track must not be able to reach it @@ -1587,6 +1604,14 @@ func TestRun_ConflictOnAReusedArtifactExplainsTheLink(t *testing.T) { assert.Contains(t, err.Error(), ".datarobot", "where that choice is recorded") assert.Contains(t, err.Error(), "wl-owner", "the workload that already has it") assert.Contains(t, err.Error(), "workloadId: wl-owner", "a line the reader can paste") + + // The other branch of the same choice used to end at deleting the link + // directory, which takes the code catalog and the last-synced version with + // it for the sake of one field. + assert.Contains(t, err.Error(), "dr artifact code init --force", + "the second way out has to be a command") + assert.NotContains(t, err.Error(), "delete", + "recovery must never end at deleting the state directory") } // A 409 on a linked project is just as likely to be a duplicate workload @@ -2926,8 +2951,10 @@ func TestDeployable_DeadWorkloadRemediesSayTheEndpointChanges(t *testing.T) { assert.Contains(t, err.Error(), "'dr workload delete 68b0c1d2e3f4a5b6c7d8e9f0 --dir ./svc'", "the remedy this note qualifies, with the --dir the run was given") - assert.Contains(t, err.Error(), ", then deploy again. The replacement keeps the name", + assert.Contains(t, err.Error(), ", then deploy again", "then, not and: the imperative must not read as another thing the delete does") + assert.Contains(t, err.Error(), "one step. The replacement keeps the name", + "the cost lands last, after both ways of paying it") assert.Contains(t, err.Error(), "new endpoint URL") assert.Contains(t, err.Error(), "anything calling the old URL has to be pointed at the new one") assert.NotContains(t, err.Error(), "under the same name") @@ -2935,6 +2962,47 @@ func TestDeployable_DeadWorkloadRemediesSayTheEndpointChanges(t *testing.T) { } } +// The message that gets there first. deployable speaks for the run after the +// workload is already stuck; this one is printed at the moment it becomes +// stuck, and it used to end at "check the logs" — the dead end that made +// hand-deleting the workloadId the folk remedy, and the 409 that followed a +// loop back to the line just deleted. +func TestRun_ADeployThatEndsDeadNamesTheWayOut(t *testing.T) { + for _, status := range []string{workload.WorkloadStatusErrored, workload.WorkloadStatusTerminated} { + t.Run(status, func(t *testing.T) { + dead := running("wl-new") + dead.Status = status + + install(t, fakes{ + create: func(any) (*workload.Workload, error) { return running("wl-new"), nil }, + wait: func(string, workload.Serving, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + return dead, nil + }, + }) + + _, _, err := runIn(t, unboundImageManifest, Options{NonInteractive: true}) + require.Error(t, err) + + assert.Contains(t, err.Error(), "dr workload delete wl-new", + "the refusal has to carry its own exit") + assert.Contains(t, err.Error(), "clears the binding", + "the delete owns the binding, so nobody hand-edits the file") + assert.Contains(t, err.Error(), "--recreate", "and the one-step form of the same recovery") + + // Errored is the one dead-looking state that can recover, so it is + // hedged with diagnosis first; terminated really is final. + if status == workload.WorkloadStatusErrored { + assert.Contains(t, err.Error(), "dr workload logs wl-new", "diagnosis before deletion") + } else { + assert.NotContains(t, err.Error(), "dr workload logs", + "no hedge for a state that cannot recover") + } + }) + } +} + // The third refusal giving the same advice. It is reached by a create that 409s // on a name a dead workload still holds, and the sentence it qualifies opens // "If it is this project's dead workload", so its reader loses the same URL. @@ -3009,3 +3077,36 @@ func TestRun_ImportEnvNamesTheValuesWrittenInTheClear(t *testing.T) { assert.NotContains(t, stderr, "OPENAI_API_KEY", "the secret is the thing being protected") assert.Equal(t, []string{"REGION"}, result.Env.Literals) } + +// The remedy has to reach the project it is about. A run against a manifest one +// level down names a delete that would find nothing without the flag. +func TestRun_TheDeadEndRemedyCarriesTheDirFlag(t *testing.T) { + dead := running("wl-new") + dead.Status = workload.WorkloadStatusErrored + + install(t, fakes{ + create: func(any) (*workload.Workload, error) { return running("wl-new"), nil }, + wait: func(string, workload.Serving, time.Duration, time.Duration, + func(*workload.Workload), + ) (*workload.Workload, error) { + return dead, nil + }, + }) + + root := t.TempDir() + app := filepath.Join(root, "service") + require.NoError(t, os.MkdirAll(app, 0o755)) + writeManifest(t, app, unboundImageManifest) + require.NoError(t, os.WriteFile(filepath.Join(app, "Dockerfile"), + []byte("FROM scratch\nEXPOSE 8080\n"), 0o600)) + + t.Chdir(root) + + var stderr bytes.Buffer + + _, err := Run(Options{Dir: app, Stderr: &stderr, NonInteractive: true}) + require.Error(t, err) + + assert.Contains(t, err.Error(), "dr workload delete wl-new --dir ", + "a delete without --dir would run in the parent and find nothing") +} diff --git a/internal/workload/wapi/relink.go b/internal/workload/wapi/relink.go new file mode 100644 index 000000000..607070d81 --- /dev/null +++ b/internal/workload/wapi/relink.go @@ -0,0 +1,113 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package wapi + +import ( + "fmt" + "time" + + "github.com/datarobot/cli/internal/version" +) + +// Relink points an already-linked project at a different artifact, in place. +// +// It exists so that "which artifact does this directory push to" has an answer +// that is a command. Before it, the only way to change one was to delete the +// state directory, which every message that needed to say so duly recommended — +// and which takes the project's ignore file and its whole history with it, for +// a change to one field. +// +// What it does not do is pretend the old artifact's code store still applies. +// CatalogID and LastSyncedVersionID come from the artifact being linked to, the +// same way Initialize takes them from a fresh one, because they describe that +// artifact's code store rather than this directory's history with the last one. +// Carrying them across would have the next sync diff against a catalog the new +// artifact never had, and report every file as unchanged when none of them are +// there. +// +// The BASE manifest goes back to empty for the same reason: it records the tree +// last synced into the artifact being left behind. +// +// LastBuiltVersionID is dropped rather than carried, which leaves imageStale +// with nil and costs the next deploy a rebuild. That is the answer, not a +// side effect: the id records a code version in the catalog of the artifact +// being left, so comparing it against whatever the new artifact is serving +// compares two different lineages. imageStale already treats every answer it +// cannot establish as stale, and this is one of them. +// +// CreatedAt is kept. It records when this directory became a linked project, +// which re-pointing it does not change. CLIVersion is stamped fresh, because it +// records which build last wrote the file, and this write is the one that did. +// +// Those six are the whole of Config. A field added to it later has to be +// settled here by name too: this literal is the file after the write, so a +// field left out of it is a field cleared, whether or not that was meant. +// +// It is therefore not idempotent, and callers must not hand it the artifact the +// project is already linked to. Resetting the baseline is the whole point when +// the artifact changes and pure loss when it does not: with BASE empty and +// LastSyncedVersionID cleared, sync classifies a file that exists on both sides +// with different bytes as ADD_CONFLICT rather than LOCAL_MODIFIED, so the next +// run renames the local edit to .LOCAL. and downloads the +// remote copy over it — an edit that would otherwise have uploaded cleanly. +// The check belongs to the caller because only it knows whether being handed +// the current id is a mistake or a deliberate baseline reset; the one caller +// there is, `artifact code init --force`, treats it as the former and no-ops. +// +// Returns ErrNotInitialized when there is no link to move. Relinking is not a +// way to create one: Initialize is, and it can say what a fresh link needs. +func Relink(projectDir string, opts InitOptions) error { + if err := validateInitOptions(opts); err != nil { + return err + } + + if !Exists(projectDir) { + return ErrNotInitialized + } + + // Read before write, so a corrupted config is reported as itself rather + // than silently replaced by this one. It also carries CreatedAt across. + previous, err := LoadConfig(projectDir) + if err != nil { + return err + } + + now := time.Now().UTC() + + cfg := Config{ + ArtifactID: opts.ArtifactID, + CatalogID: stringPtr(opts.CatalogID), + LastSyncedVersionID: stringPtr(opts.LastSyncedVersionID), + CreatedAt: previous.CreatedAt, + CLIVersion: version.Version, + } + + if err := writeConfig(projectDir, cfg); err != nil { + return fmt.Errorf("point %s at artifact %s: %w", projectDir, opts.ArtifactID, err) + } + + if err := writeManifest(projectDir, Manifest{Version: ManifestVersion}); err != nil { + return err + } + + return AppendHistory(projectDir, HistoryEntry{ + "ts": now.Format(time.RFC3339), + "op": "relink", + "artifact": opts.ArtifactID, + "from": previous.ArtifactID, + "catalog": stringPtr(opts.CatalogID), + "duration": "0.0s", + }) +} diff --git a/internal/workload/wapi/relink_test.go b/internal/workload/wapi/relink_test.go new file mode 100644 index 000000000..61643c99a --- /dev/null +++ b/internal/workload/wapi/relink_test.go @@ -0,0 +1,153 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package wapi + +import ( + "os" + "strings" + "testing" + + "github.com/datarobot/cli/internal/workload/ignore" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func linked(t *testing.T) string { + t.Helper() + + tmp := t.TempDir() + require.NoError(t, Initialize(tmp, InitOptions{ + ArtifactID: "68b0aaaa0000000000000001", + CatalogID: "68b0bbbb0000000000000002", + LastSyncedVersionID: "68b0cccc0000000000000003", + })) + + return tmp +} + +func TestRelink_PointsTheProjectAtTheNewArtifact(t *testing.T) { + tmp := linked(t) + + before, err := LoadConfig(tmp) + require.NoError(t, err) + + require.NoError(t, Relink(tmp, InitOptions{ + ArtifactID: "68b0dddd0000000000000004", + CatalogID: "68b0eeee0000000000000005", + })) + + cfg, err := LoadConfig(tmp) + require.NoError(t, err) + + assert.Equal(t, "68b0dddd0000000000000004", cfg.ArtifactID) + + // The catalog comes from the artifact being linked to, not from the one + // being left. Carrying the old one across would have the next sync diff + // against a code store the new artifact never had. + require.NotNil(t, cfg.CatalogID) + assert.Equal(t, "68b0eeee0000000000000005", *cfg.CatalogID) + assert.Nil(t, cfg.LastSyncedVersionID, "nothing has been synced into the new artifact yet") + + assert.Equal(t, before.CreatedAt, cfg.CreatedAt, + "CreatedAt records when this became a linked project, which re-pointing does not change") +} + +// The BASE records the tree last synced into the artifact being left behind. +func TestRelink_ResetsTheBaseManifest(t *testing.T) { + tmp := linked(t) + + require.NoError(t, SaveManifest(tmp, Manifest{ + Version: ManifestVersion, + Files: map[string]FileMeta{"main.py": {}}, + })) + + require.NoError(t, Relink(tmp, InitOptions{ArtifactID: "68b0dddd0000000000000004"})) + + m, err := LoadManifest(tmp) + require.NoError(t, err) + assert.Empty(t, m.Files, "the new artifact has never seen these files") +} + +// The recorded build refers to a code version in the catalog of the artifact +// being left, so it says nothing about what the new one is serving. Clearing it +// leaves imageStale with nil, which it reads as stale, and the next deploy pays +// for one rebuild rather than inheriting an image from another lineage. +func TestRelink_ClearsTheLastBuiltVersion(t *testing.T) { + tmp := linked(t) + + cfg, err := LoadConfig(tmp) + require.NoError(t, err) + + built := "68b0ffff0000000000000006" + cfg.LastBuiltVersionID = &built + require.NoError(t, SaveConfig(tmp, cfg)) + + require.NoError(t, Relink(tmp, InitOptions{ArtifactID: "68b0dddd0000000000000004"})) + + after, err := LoadConfig(tmp) + require.NoError(t, err) + assert.Nil(t, after.LastBuiltVersionID, + "an image built against the old artifact's code cannot vouch for the new one") +} + +// The whole reason this exists rather than "delete the state directory": the +// things a deletion would take with it survive. +func TestRelink_KeepsTheHistoryAndTheIgnoreFile(t *testing.T) { + tmp := linked(t) + + ignorePath := ignore.Locate(tmp) + require.NotEmpty(t, ignorePath, "Initialize drops a starter ignore file") + require.NoError(t, os.WriteFile(ignorePath, []byte("# mine\n*.tmp\n"), 0o600)) + + require.NoError(t, Relink(tmp, InitOptions{ArtifactID: "68b0dddd0000000000000004"})) + + kept, err := os.ReadFile(ignorePath) + require.NoError(t, err) + assert.Contains(t, string(kept), "*.tmp", "the user's patterns are not this command's to discard") + + history, err := os.ReadFile(historyPath(tmp)) + require.NoError(t, err) + + text := string(history) + assert.Contains(t, text, `"op":"init"`, "the log is appended to, not started over") + assert.Contains(t, text, `"op":"relink"`) + assert.Contains(t, text, "68b0aaaa0000000000000001", "the id left behind is the way back") + assert.Contains(t, text, "68b0dddd0000000000000004") +} + +// Relinking is not a way to create a link. Initialize is, and it can say what +// a fresh one needs. +func TestRelink_RefusesAnUnlinkedDirectory(t *testing.T) { + err := Relink(t.TempDir(), InitOptions{ArtifactID: "68b0dddd0000000000000004"}) + require.Error(t, err) + assert.ErrorIs(t, err, ErrNotInitialized) +} + +func TestRelink_RefusesAnArtifactIDItCannotUse(t *testing.T) { + err := Relink(linked(t), InitOptions{}) + require.Error(t, err) + assert.True(t, strings.Contains(err.Error(), "artifactId") || + strings.Contains(err.Error(), "ArtifactID"), "name the field that is missing: %v", err) +} + +// A config that cannot be read is reported as itself rather than silently +// replaced, because it may not be this project's state at all. +func TestRelink_ReportsACorruptedConfig(t *testing.T) { + tmp := linked(t) + require.NoError(t, os.WriteFile(configPath(tmp), []byte("{not json"), 0o600)) + + err := Relink(tmp, InitOptions{ArtifactID: "68b0dddd0000000000000004"}) + require.Error(t, err) +} diff --git a/smoke_test_scripts/run_workload_smoke_test.sh b/smoke_test_scripts/run_workload_smoke_test.sh index d98b0f425..0b1ea1dde 100755 --- a/smoke_test_scripts/run_workload_smoke_test.sh +++ b/smoke_test_scripts/run_workload_smoke_test.sh @@ -29,7 +29,7 @@ RESULTS="" if [[ $# -gt 0 ]]; then scenarios=("$@") else - scenarios=(a b c artifact) + scenarios=(a b c e artifact) if [[ "${WORKLOAD_SMOKE_INCLUDE_D:-0}" == "1" ]]; then scenarios+=(d) fi @@ -43,6 +43,7 @@ scenario_script() { b|B) printf '%s/RAPTOR-19533-B-sweep.sh' "$WL_DIR" ;; c|C) printf '%s/RAPTOR-19533-C-rebind.sh' "$WL_DIR" ;; d|D) printf '%s/RAPTOR-19533-D-built.sh' "$WL_DIR" ;; + e|E) printf '%s/RAPTOR-19729-E-recovery.sh' "$WL_DIR" ;; artifact) printf '%s/artifact-lifecycle.sh' "$WL_DIR" ;; *) echo "❌ unknown scenario: $1" >&2; return 1 ;; esac diff --git a/smoke_test_scripts/workload/RAPTOR-19729-E-recovery.sh b/smoke_test_scripts/workload/RAPTOR-19729-E-recovery.sh new file mode 100755 index 000000000..e27247a43 --- /dev/null +++ b/smoke_test_scripts/workload/RAPTOR-19729-E-recovery.sh @@ -0,0 +1,161 @@ +#!/usr/bin/env bash +# Ticket: RAPTOR-19729 +# Scenario E — recovery never ends at "delete the file". +# +# The loop this guards: nothing told the user to remove workloadId, removing +# it by hand made the create hit 409, and the 409 advised setting the exact +# line just deleted. Deleting .datarobot/ was the only escape people found, +# and it takes the code catalog with it. +# +# Asserts the two halves of the fix against a live tenant: every refusal on a +# dead workload names a command rather than a file, and --recreate performs +# that command inside the deploy. +# +# ~8 min. + +# shellcheck shell=bash +set -euo pipefail +source "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +wl::init_env +wl::register_cleanup + +wl::start_timer "E: recovery never ends at the file" + +# --- E.1 A workload that cannot come up ------------------------------------ +# An image that exits immediately is the cheapest way to reach `errored` +# without waiting out a build. +work="$WL_SCRATCH/project" +mkdir -p "$work" +cat > "$work/.datarobot.yaml" < "$WL_SCRATCH/deploy-err.txt" + +# The create writes the binding before the wait, so any run that got as far as +# a workload leaves an id in the file. Register it before deciding anything: +# a scenario that bails still has to clean up what it made. +wl_id="$(grep -oE '[0-9a-f]{24}' "$work/.datarobot.yaml" | head -1)" +if [[ -n "$wl_id" ]]; then + wl::register_workload "$wl_id" +fi + +# Both of these were skips, which the runner scores as a pass — a green E that +# asserted nothing, --recreate included. Neither is a tolerable outcome: this +# scenario exists to exercise recovery from a dead workload, and a run that +# never reaches one has not tested the thing it is here to test. +if [[ "$up_rc" -eq 0 ]]; then + wl::fail "the workload came up; this scenario needs an image that exits, and busybox no longer does" +fi + +if [[ -z "$wl_id" ]]; then + wl::fail "the deploy failed before a workload existed; nothing to recover from (see deploy-err.txt)" +fi + +# --- E.2 The refusal carries its own exit ---------------------------------- +# It used to end at "check the logs", which is where recovery ran out. + +grep -q "dr workload delete" "$WL_SCRATCH/deploy-err.txt" \ + || wl::fail "the dead-deploy message must name the delete that clears the way" +wl::pass "the dead-deploy message names 'dr workload delete'" + +grep -q -- "--recreate" "$WL_SCRATCH/deploy-err.txt" \ + || wl::fail "the dead-deploy message must name the one-step recovery" +wl::pass "the dead-deploy message names --recreate" + +# The regression that defines this ticket: no message may end at removing a +# file or a directory the user has to find themselves. +# The strings this guards against were "delete /abs/path/.datarobot/workload" +# and "Delete /abs/path/.datarobot/workload to re-init." — a path sits between +# the verb and the dot, so a pattern that expects them adjacent matches neither, +# and grep -E is case-sensitive besides. Anchoring on the trailing slash keeps +# it off ".datarobot.yaml", which several healthy messages do name. +wl::assert_absent "$WL_SCRATCH/deploy-err.txt" \ + '([Dd]elete|[Rr]emove|rm -rf).{0,80}\.datarobot/|[Dd]elete the (file|state)' \ + "recovery never ends at deleting state" + +# --- E.3 A second deploy refuses the same way ------------------------------ +# deployable() speaks for the run once the workload is already stuck. Both +# messages have to agree, or the second run reopens the loop the first closed. +wl::dr_capture workload up --dir "$work" --yes +printf '%s\n%s\n' "$WL_OUT" "$WL_ERR" > "$WL_SCRATCH/second-err.txt" + +grep -q "dr workload delete" "$WL_SCRATCH/second-err.txt" \ + || wl::fail "the errored refusal must name the delete too" +wl::pass "the errored refusal agrees with the deploy that produced it" + +# --- E.4 --dry-run --recreate deletes nothing ------------------------------ +# The delete happens before the plan is printed, so a dry run that deleted +# would be a dry run that mutated. +wl::dr_capture workload up --dir "$work" --yes --recreate --dry-run --output-format json +wl::assert_cmd_ok "$WL_RC" "$WL_OUT" "$WL_ERR" "workload up --recreate --dry-run" + +# Held now: the next wl:: call reuses WL_OUT. +preview_json="$WL_OUT" + +still="$(wl::dr_jq '.status' workload get "$wl_id" 2>/dev/null || echo gone)" +[[ "$still" != "gone" ]] \ + || wl::fail "--dry-run --recreate deleted the workload" +wl::pass "--dry-run --recreate left the workload alone (status: $still)" + +# A preview has to predict the run it previews. Planning against the workload +# that is about to be deleted described a roll onto something the very next +# check refuses, while the hint above it said a create would happen. +previewed="$(printf '%s' "$preview_json" | jq -r '.up.action')" +[[ "$previewed" == "created" ]] \ + || wl::fail "--dry-run --recreate planned '$previewed'; the delete leaves a create" +wl::pass "--dry-run --recreate plans the create the real run performs" + +# --- E.5 --recreate performs the recovery ---------------------------------- +# The workload is deleted, the binding goes with it, and the run continues +# into the create — the whole loop, closed by one command. +wl::dr_capture workload up --dir "$work" --yes --recreate +printf '%s\n%s\n' "$WL_OUT" "$WL_ERR" > "$WL_SCRATCH/recreate-err.txt" + +grep -qE "Deleting the (errored|terminated) workload" "$WL_SCRATCH/recreate-err.txt" \ + || wl::fail "--recreate must say what it deleted" +wl::pass "--recreate deleted the dead workload" + +new_id="$(grep -oE '[0-9a-f]{24}' "$work/.datarobot.yaml" | head -1)" +if [[ -n "$new_id" && "$new_id" != "$wl_id" ]]; then + wl::register_workload "$new_id" + wl::pass "--recreate rebound the manifest to a new workload ($new_id)" +else + wl::fail "the manifest still names $wl_id after --recreate" +fi + +# The state directory survives: the catalog and last-synced version are what +# deleting .datarobot/ used to throw away. +if [[ -d "$work/.datarobot/workload" ]]; then + wl::pass "the project's state directory survived the recovery" +else + wl::fail "--recreate must not discard the code catalog" +fi + +wl::stop_timer diff --git a/smoke_test_scripts/workload/TICKETS.md b/smoke_test_scripts/workload/TICKETS.md index 92b680e75..84ff9c4f3 100644 --- a/smoke_test_scripts/workload/TICKETS.md +++ b/smoke_test_scripts/workload/TICKETS.md @@ -11,6 +11,7 @@ scenario script (grep with `rg '^# Ticket:' smoke_test_scripts/workload`). | B — account sweep | `RAPTOR-19533-B-sweep.sh` | RAPTOR-19533 | binding works against every workload on the account | | C — re-bind tuned | `RAPTOR-19533-C-rebind.sh` | RAPTOR-19533 | re-bind preserves live tuning; FileExists guard; delete-rebind restore | | D — built-workload rebuild | `RAPTOR-19533-D-built.sh` | RAPTOR-19533 | no ErrImagePull after re-bind; platform rebuilds from imageBuildConfig | +| E — recovery from a dead workload | `RAPTOR-19729-E-recovery.sh` | RAPTOR-19729 | every refusal names a command, not a file; `--recreate` recovers without discarding state | | Artifact lifecycle | `artifact-lifecycle.sh` | none — basic acceptance | `dr artifact` create/get/list/code sync/versions/del CLI-side state | ## Adding a scenario for a new ticket