Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Expand Down
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <path>`: 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 `<path>.LOCAL.<timestamp>`, 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 <id> --dir <path>`, 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 <path>` 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.
Expand All @@ -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.
75 changes: 74 additions & 1 deletion cmd/artifact/code/init/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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),
}
})
Expand All @@ -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)
}

Expand All @@ -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)
Expand All @@ -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 <path>.LOCAL.<timestamp>, 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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] This returns before printRelinked and initResult has no field for previous, so --force --output-format json never emits the id it replaced. The comment a few lines up says that id is the only thing left that remembers where the link came from, and docs/commands/artifact.md says the same. A nullable previousArtifactId on initResult would keep both paths honest.

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 {
Expand Down
Loading
Loading