[RAPTOR-19721] feat(workload): take the .env flags on up as well as config

CHANGELOG.md

@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 - `dr workload config --import-env` adds the `.env` variables a manifest does not declare yet, and changes nothing else. Until now `.env` was read exactly once, when setup wrote `.datarobot.yaml`, so a variable added afterwards never reached the container and no command said so: the only route was a hand edit of the manifest plus a hand-made credential for anything secret. The import is additive and one-directional. A name the file already carries keeps its value, because the manifest is what is deployed and `.env` is a local copy allowed to drift; a name dropped from `.env` is left alone, because removing a line from your own file says nothing about what the workload should run. Secrets are stored as credentials the way setup stores them, and the file is edited in place, so comments and hand-tuned keys survive. It is opt-in, and exclusive with `--skip-env` and with every setup answer: re-reading `.env` on every run would carry whatever it grew since into a deploy nobody asked to change, and a flag an import cannot apply is refused rather than dropped. The edit is refused outright, before any credential is created, for a manifest with nowhere to put the result: one that names its artifact by id, one where any step from the artifact down to the variable list is shared through a YAML anchor, alias or merge key, one whose `environmentVars` is not a list, and one holding more than a single YAML document. The refusal is the edit itself, rendered and thrown away, so it cannot disagree with the write it guards. The edited file is then parsed and validated before it reaches the disk.
 - `dr workload config --update-env` brings the variables a manifest already declares back in line with `.env`: a literal is rewritten in the file, and the credential behind a secret is re-sent so a rotated key reaches the workload without a new credential, a new id or an edit to the manifest. It is the counterpart of `--import-env` and deliberately a separate flag, because it is the opposite act: one adds a name and never touches a value, the other touches values and never adds a name. Pass both to do both. A secret is re-sent without being compared first, because it cannot be: the platform never returns a stored value, so nothing can tell a rotated key from an untouched one, and a run reporting "no change" for a key you had just rotated would be worse than one request too many. A re-send reaches a running workload only when its containers restart, because a container reads its credentials at startup and `dr workload up` finds the manifest unchanged and does nothing, so the run says which commands make the new value the one being served. A re-send that fails is named, because it leaves the workload serving the value it already had and nothing downstream would notice; under `--output-format json` there is no stderr for that line to have used, so the envelope carries the counts of what was re-sent and what was not. A dry run says how many secrets it would re-send rather than staying silent about the one part of the flag that reaches the tenant. A run that only re-sent secrets reports that the manifest was not changed, because it was not: the value went to the credential store and the file naming it is byte for byte what it was. Only a credential this project created is ever re-sent, checked by name before anything is sent: the store is tenant-wide, so an id pasted by hand may name one other workloads read, and a flag about this project's `.env` has no business overwriting it. A reference to any field other than the one the CLI writes is refused for the same reason.
+- `dr workload up --import-env` and `dr workload up --update-env` do the same two edits as part of a deploy, so adding a variable or rotating a key is one command rather than two. They are the same act `up` already performs on its first run, where a project with no manifest gets the wizard, which reads `.env`, classifies it and mints the credentials: without these the second run was the only one that could not, which made the file a deploy reads something you had to leave the command to change. Both stay opt-in, which is what keeps a deploy a function of the committed repo: a run without them reads nothing but the manifest, so a fresh CI clone with no `.env` deploys exactly what your working copy deploys. The edit runs before the plan, so what the plan shows and what the deploy carries is the file as edited, and `--dry-run` previews the edit without writing it, saying outright that the plan below is for the manifest as it stands. A re-sent secret is the one edit a deploy cannot finish: it changes no file, so a deploy with nothing else to do replaces no container, and the run says so and names the restart rather than reporting the workload as up to date and leaving it at that.
 - `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 named by the manifest rather than by you 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 manifest-named `delete`: that variable is set once across a pipeline, and deleting something nobody named is not what it was set for.

cmd/workload/up/cmd.go

@@ -69,6 +69,54 @@ type upResult struct {
 	Action     string      `json:"action"`
 	Locked     bool        `json:"locked"`
 	Plan       up.PlanJSON `json:"plan"`
+	// Env is what --import-env and --update-env did before the plan was
+	// computed. A rotation edits no file and shows in no plan, so without
+	// these a run that re-sent a secret is indistinguishable from one that
+	// did nothing at all.
+	Env envJSON `json:"env"`
+}
+
+// envJSON is the .env re-entry's side of a deploy.
+type envJSON struct {
+	KeysAdded      int `json:"keysAdded"`
+	ValuesUpdated  int `json:"valuesUpdated"`
+	SecretsRotated int `json:"secretsRotated"`
+	SecretsFailed  int `json:"secretsNotRotated"`
+	SecretsPending int `json:"secretsPending"`
+}
+
+// warnSecretStillServing covers the one thing --update-env can do that a
+// deploy cannot finish: re-send a secret.
+//
+// The credential store takes the new value immediately, but a container reads
+// its credentials when it starts, so the workload keeps serving the old one
+// until it is replaced. A deploy that had something else to do replaces it on
+// the way past; a deploy that found nothing else leaves the rotation sitting
+// in the store, under a summary that says the workload is up to date. It is,
+// about the manifest, which is why this says the other half out loud.
+func warnSecretStillServing(stderr io.Writer, result up.Result, f flags) {
+	if f.dryRun || result.Env.SecretsRotated == 0 || result.Action != up.ActionUnchanged {
+		return
+	}
+
+	dir := manifest.DirFlag(f.dir)
+
+	fmt.Fprintf(stderr,
+		"\n  %s %d re-sent %s reached the credential store, and this deploy replaced no container, "+
+			"so the workload still serves the value it started with.\n    Restart it to pick %s up:\n"+
+			"      dr workload stop --yes%s\n      dr workload start --yes%s\n",
+		tui.WarnStyle.Render("!"), result.Env.SecretsRotated,
+		plural(result.Env.SecretsRotated, "secret", "secrets"),
+		plural(result.Env.SecretsRotated, "it", "them"), dir, dir)
+}
+
+// plural picks the word for count, the way the wizard's own reporting does.
+func plural(count int, one, many string) string {
+	if count == 1 {
+		return one
+	}
+
+	return many
 }
 
 // buildID is the envelope's build reference: the id when a build ran, null
@@ -82,12 +130,14 @@ func buildID(id string) *string {
 }
 
 type flags struct {
-	dir    string
-	yes    bool
-	dryRun bool
-	detach bool
-	lock   bool
-	force  bool
+	dir       string
+	yes       bool
+	dryRun    bool
+	detach    bool
+	lock      bool
+	force     bool
+	importEnv bool
+	updateEnv bool
 
 	// bindingFlags exist only to be refused. Cobra's own "unknown flag"
 	// message would leave the user guessing where binding lives, and these
@@ -202,6 +252,18 @@ func addFlags(cmd *cobra.Command, f *flags, poll *pollflags.Set) {
 	cmd.Flags().BoolVar(&f.force, "force-build", false,
 		"Rebuild the image even when the working tree matches what was last synced.")
 
+	// 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
+	// deploy stays a function of the committed repo, since neither flag does
+	// anything unless it is passed.
+	cmd.Flags().BoolVar(&f.importEnv, "import-env", false,
+		"Before deploying, add the .env variables the manifest does not declare yet. "+
+			"Secrets are stored as credentials, the same way setup stores them.")
+	cmd.Flags().BoolVar(&f.updateEnv, "update-env", false,
+		"Before deploying, bring the variables the manifest already declares back in line with .env: "+
+			"a literal is rewritten and the credential behind a secret is re-sent. A re-sent secret reaches "+
+			"the containers this deploy replaces; if the deploy has nothing else to do, it will not replace them.")
+
 	cmd.Flags().StringVar(&f.workloadID, "workload-id", "", "")
 	cmd.Flags().StringVar(&f.name, "name", "", "")
 	_ = cmd.Flags().MarkHidden("workload-id")
@@ -243,6 +305,8 @@ func run(cmd *cobra.Command, f flags, poll pollflags.Set, format outputformat.Ou
 		Lock:           f.lock,
 		Confirm:        rollConfirm(cmd, yes),
 		ForceBuild:     f.force,
+		ImportEnv:      f.importEnv,
+		UpdateEnv:      f.updateEnv,
 		PollInterval:   poll.Interval,
 		PollTimeout:    poll.Timeout,
 		Stderr:         cmd.ErrOrStderr(),
@@ -411,9 +475,18 @@ func render(cmd *cobra.Command, f flags, format outputformat.OutputFormat, resul
 			Action:     result.Action,
 			Locked:     result.Locked,
 			Plan:       result.Plan.JSON(),
+			Env: envJSON{
+				KeysAdded:      result.Env.KeysAdded,
+				ValuesUpdated:  result.Env.ValuesUpdated,
+				SecretsRotated: result.Env.SecretsRotated,
+				SecretsFailed:  result.Env.SecretsFailed,
+				SecretsPending: result.Env.SecretsPending,
+			},
 		})
 	}
 
+	warnSecretStillServing(cmd.ErrOrStderr(), result, f)
+
 	draft := draftIsServing(f, result, failed)
 
 	if f.dryRun {

internal/workload/up/load.go

@@ -44,6 +44,10 @@ type Loaded struct {
 	// shell happened to be sitting in.
 	ProjectDir string
 
+	// Env is what a --import-env or --update-env run did to this file before
+	// it was read, zero when neither was asked for.
+	Env EnvEdit
+
 	// Manifest is the parse tree, kept so later stages can ask it questions
 	// without re-reading the file.
 	Manifest *manifest.Manifest

internal/workload/up/load_test.go

@@ -17,9 +17,11 @@ package up
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/datarobot/cli/internal/workload/manifest"
+	"github.com/datarobot/cli/internal/workload/wizard"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -261,3 +263,87 @@ func TestLoad_ADirectoryAtThePathIsNotAManifest(t *testing.T) {
 	require.Error(t, err)
 	assert.ErrorIs(t, err, ErrNoManifest)
 }
+
+// The flag exists because the first run of this command already reads .env:
+// with no manifest `up` is the wizard. The edit has to happen before the file
+// is read, or the deploy would carry the version from before it.
+func TestLoad_ImportEnvEditsTheManifestBeforeReadingIt(t *testing.T) {
+	dir := t.TempDir()
+	path := writeManifest(t, dir, boundManifest)
+
+	swap(t, &runWizardFn, func(opts wizard.Options) (wizard.Result, error) {
+		assert.True(t, opts.ImportEnv)
+		assert.Equal(t, dir, opts.Dir, "the edit lands next to the manifest being deployed")
+
+		// What the real wizard does: the variable is in the file by the time
+		// it returns.
+		edited := strings.Replace(boundManifest,
+			"              - name: LOG_LEVEL",
+			"              - name: REGION\n                value: eu-west-1\n              - name: LOG_LEVEL", 1)
+		require.NoError(t, os.WriteFile(path, []byte(edited), 0o600))
+
+		return wizard.Result{Path: path, Action: wizard.ActionUpdated, EnvKeysListed: 1}, nil
+	})
+
+	loaded, err := load(dir, Options{ImportEnv: true})
+	require.NoError(t, err)
+
+	assert.Equal(t, 1, loaded.Env.KeysAdded)
+	assert.Contains(t, loaded.Manifest.EnvVarNames(), "REGION",
+		"the deploy plans from the file as edited, not as it was")
+}
+
+// A rotation writes to the credential store and leaves the file alone, so the
+// count is the only trace of it: the plan below has nothing to show.
+func TestLoad_UpdateEnvCarriesTheRotationCount(t *testing.T) {
+	dir := t.TempDir()
+	writeManifest(t, dir, boundManifest)
+
+	swap(t, &runWizardFn, func(opts wizard.Options) (wizard.Result, error) {
+		assert.True(t, opts.UpdateEnv)
+
+		return wizard.Result{Action: wizard.ActionUnchanged, EnvSecretsRotated: 2}, nil
+	})
+
+	loaded, err := load(dir, Options{UpdateEnv: true})
+	require.NoError(t, err)
+
+	assert.Equal(t, 2, loaded.Env.SecretsRotated)
+	assert.Equal(t, 0, loaded.Env.KeysAdded)
+}
+
+// Without the flags nothing reads .env, which is what keeps a deploy a
+// function of the committed repo: a fresh CI clone has no .env at all.
+func TestLoad_WithoutTheFlagsTheWizardIsNotRun(t *testing.T) {
+	dir := t.TempDir()
+	writeManifest(t, dir, boundManifest)
+
+	swap(t, &runWizardFn, func(wizard.Options) (wizard.Result, error) {
+		t.Fatal("the deploy read .env without being asked to")
+
+		return wizard.Result{}, nil
+	})
+
+	_, err := load(dir, Options{})
+	require.NoError(t, err)
+}
+
+// A dry run promises the file is not touched, and an import that minted a
+// credential and rewrote the manifest would have changed two things the plan
+// then says it is not going to do.
+func TestLoad_DryRunPreviewsTheEdit(t *testing.T) {
+	dir := t.TempDir()
+	writeManifest(t, dir, boundManifest)
+
+	swap(t, &runWizardFn, func(opts wizard.Options) (wizard.Result, error) {
+		assert.True(t, opts.DryRun, "the preview must reach the wizard, or it writes")
+
+		return wizard.Result{Action: wizard.ActionPlanned, EnvKeysListed: 1}, nil
+	})
+
+	loaded, err := load(dir, Options{ImportEnv: true, DryRun: true})
+	require.NoError(t, err)
+
+	assert.Equal(t, 1, loaded.Env.KeysAdded)
+	assert.NotContains(t, loaded.Manifest.EnvVarNames(), "REGION")
+}