carlos / platform Public

cli: deploy — no duplicate banner/shipped line, deploy's own refusal, validate -host under -channel

Three review fixes on carlos deploy:

- runShipAPI gains a printBanner bool so deploy can suppress its own
  "using ..." line (already printed once, before the instance list) and
  no longer prints a second, contradictory "shipped" line of its own —
  runShipAPI's shipped/already-shipped wording is the one true account.
- deploy no longer borrows instances_cli.go's requireAPISession, whose
  hardcoded refusal text names a nonexistent "carlos deploy" sub-verb and
  unrelated route advice in direct-bucket mode; requireDeployAPISession
  gives deploy its own message pointing at ship + promote.
- -channel no longer bypasses host validation: filterChannelHosts checks
  a given -host against the instance list up front, so a typo'd host
  errors before ship/promote run instead of surfacing afterwards as "no
  instance follows <channel>" with the host never named.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell 2 months ago ada737389e3be7603cf8a4d400419bbcd4f77a4a parent 0b59e2b

4 files changed, +121 −16

apimode_test.go +5 −5
cli.go +13 −3
deploy.go +69 −8
deploy_test.go +34 −0

diff --git a/apimode_test.go b/apimode_test.go

index 7fbff641b..0f0360882 100644

--- a/apimode_test.go

+++ b/apimode_test.go

@@ -1084,7 +1084,7 @@ func TestShipAPIStaticNamesSiteTarGz(t *testing.T) {

sess := newTestSession(ts.URL)

fs, acctRaw, acctIDRaw := newShipFlagSet(t, apiTestAccount, siteDir)

- if err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, "titesite", "abc1234", "static", "", nil); err != nil {

+ if err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, "titesite", "abc1234", "static", "", nil, true); err != nil {

t.Fatalf("runShipAPI: %v", err)

}

@@ -1131,7 +1131,7 @@ func TestShipAPIStaticCleansUpTempFileOnServerError(t *testing.T) {

sess := newTestSession(ts.URL)

fs, acctRaw, acctIDRaw := newShipFlagSet(t, apiTestAccount, siteDir)

- if err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, "titesite", "abc1234", "static", "", nil); err == nil {

+ if err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, "titesite", "abc1234", "static", "", nil, true); err == nil {

t.Fatal("runShipAPI against a failing server must error")

}

@@ -1197,7 +1197,7 @@ func TestShipAPIMultiFileGoodBasenames(t *testing.T) {

sess := newTestSession(ts.URL)

fs, acctRaw, acctIDRaw := newShipFlagSet(t, apiTestAccount, fileArm, fileAmd)

- if err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, "titogo", "abc1234", "binary", "linux-arm64", nil); err != nil {

+ if err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, "titogo", "abc1234", "binary", "linux-arm64", nil, true); err != nil {

t.Fatalf("runShipAPI: %v", err)

}

@@ -1239,7 +1239,7 @@ func TestShipAPIMultiFileBadBasenameRefused(t *testing.T) {

sess := newTestSession(ts.URL)

fs, acctRaw, acctIDRaw := newShipFlagSet(t, apiTestAccount, fileGood, fileBad)

- err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, "titogo", "abc1234", "binary", "linux-arm64", nil)

+ err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, "titogo", "abc1234", "binary", "linux-arm64", nil, true)

if err == nil {

t.Fatal("runShipAPI with a bad basename must error")

}

@@ -1274,7 +1274,7 @@ func TestShipAPIMissingArtifactRefusedBeforeDialing(t *testing.T) {

sess := newTestSession(ts.URL)

fs, acctRaw, acctIDRaw := newShipFlagSet(t, apiTestAccount, missing)

- err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, "titogo", "abc1234", "binary", "linux-arm64", nil)

+ err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, "titogo", "abc1234", "binary", "linux-arm64", nil, true)

if err == nil {

t.Fatal("runShipAPI with a missing artifact path must error")

}

diff --git a/cli.go b/cli.go

index 78682b327..ae236a9cd 100644

--- a/cli.go

+++ b/cli.go

@@ -1470,7 +1470,7 @@ func runShip(args []string) {

// branch defers the cleanup of a scratch tarball up to 256MiB).

// Evaluated as an argument, the refusal lands before runShipAPI's

// body has registered a defer at all.

- if err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, *app, *ver, *kind, *target, capsSet); err != nil {

+ if err := runShipAPI(sess, apiAccount(acctRaw, acctIDRaw), fs, *app, *ver, *kind, *target, capsSet, true); err != nil {

log.Fatal(err)

}

return

@@ -1589,7 +1589,15 @@ var shipTargetRe = regexp.MustCompile(`^[a-z0-9]+-[a-z0-9]+$`)

// function — including the static kind's scratch temp file cleanup below,

// up to 256MiB per invocation. Returning lets that defer actually run on

// every exit path, fatal or not.

-func runShipAPI(sess *apiSession, acct apiAcct, fs *flag.FlagSet, app, ver, kind, target string, capsSet *caps.Set) error {

+//

+// printBanner controls only the "using <console> as <you>" line: true for

+// runShip's own call (unchanged), false for a caller — `carlos deploy` —

+// that already printed its own banner earlier in the same run (before its

+// instance-list call, so a listing failure still names the console) and

+// would otherwise print it twice. The "shipped"/"already shipped" line

+// below is never suppressed; it is the one true account of what ship did,

+// and deploy relies on that rather than printing its own.

+func runShipAPI(sess *apiSession, acct apiAcct, fs *flag.FlagSet, app, ver, kind, target string, capsSet *caps.Set, printBanner bool) error {

files := map[string]string{}

if kind == "static" {

if fs.NArg() != 1 {

@@ -1644,7 +1652,9 @@ func runShipAPI(sess *apiSession, acct apiAcct, fs *flag.FlagSet, app, ver, kind

}

- printUsing(stdout, sess)

+ if printBanner {

+ printUsing(stdout, sess)

+ }

res, err := sess.ship(context.Background(), acct, app, ver, kind, files, capsSet)

if err != nil {

return err

diff --git a/deploy.go b/deploy.go

index 0be8674fd..1aa54df07 100644

--- a/deploy.go

+++ b/deploy.go

@@ -55,9 +55,9 @@ func runDeploy(args []string) {

log.Fatal(err)

}

- sess := requireAPISession("deploy", *consoleFlag) // API-mode only: member verb

+ sess := requireDeployAPISession(*consoleFlag) // API-mode only: member verb

acct := apiAccount(acctRaw, acctIDRaw)

- printUsing(stdout, sess)

+ printUsing(stdout, sess) // printed HERE, once: runShipAPI's own banner is suppressed below

ctx := context.Background()

recs, _, err := sess.instanceList(ctx, acct, *app)

if err != nil {

@@ -69,11 +69,14 @@ func runDeploy(args []string) {

// Explicit channel: no ambiguity to resolve. Watch the instances that

// follow it (none is fine — promote still lands; the watch is skipped

// with a note, since there is nothing that will serve this channel).

+ // -host is still validated against the instance list even here: a

+ // typo'd host must be its own error, not silently absorbed into "no

+ // instance follows this channel" once ship and promote have already

+ // run (finding 3, task-9 review).

channel = *channelFlag

- for _, r := range recs {

- if r.Channel == channel && (*hostFlag == "" || r.Host == *hostFlag) {

- hosts = append(hosts, r.Host)

- }

+ hosts, err = filterChannelHosts(recs, channel, *hostFlag)

+ if err != nil {

+ log.Fatalf("deploy: %v", err)

}

} else {

channel, hosts, err = resolveDeployChannel(recs, *hostFlag)

@@ -81,10 +84,12 @@ func runDeploy(args []string) {

log.Fatalf("deploy: %v", err)

}

- if err := runShipAPI(sess, acct, fs, *app, *ver, *kind, *target, capsSet); err != nil {

+ // printBanner=false: the "using ..." line already printed above, before

+ // the instance list — so a listing failure still names the console —

+ // and runShipAPI would otherwise print it a second time.

+ if err := runShipAPI(sess, acct, fs, *app, *ver, *kind, *target, capsSet, false); err != nil {

log.Fatalf("deploy: ship: %v", err)

}

- fmt.Fprintf(stdout, "%s %s %s\n", colorFor(stdout).ok("shipped"), *app, *ver)

res, err := sess.promote(ctx, acct, *app, *ver, channel)

if err != nil {

log.Fatalf("deploy: promote: %v", err) // a stable 403's console_url surfaces in err

@@ -107,6 +112,29 @@ func runDeploy(args []string) {

}

+// requireDeployAPISession resolves signed-in mode for deploy, refusing

+// direct-bucket mode with deploy's OWN message — not instances_cli.go's

+// requireAPISession, whose hardcoded text ("carlos instances %s works

+// through a console... use carlos add instead") names a sub-verb of a

+// different command and advice about routes, neither of which applies to

+// deploy (task-9 review, finding 2). Unlike ship/promote, deploy has no

+// direct-bucket body at all — it is pure console composition — so the

+// refusal also has nowhere to fall back to but naming the two verbs that DO

+// still work in that mode.

+func requireDeployAPISession(consoleFlag string) *apiSession {

+ sess, err := resolveMode(consoleFlag)

+ if err != nil {

+ log.Fatal(err)

+ }

+ if sess == nil {

+ log.Fatal("carlos deploy works through a console, and this shell is in direct bucket mode\n" +

+ "(CARLOS_DEPLOYMENT_BUCKET/CARLOS_DEPLOYMENT_DIR is set). deploy is a member verb with no\n" +

+ "direct-bucket equivalent; unset the store env and run: carlos auth login -console <url>,\n" +

+ "or use carlos ship + carlos promote, which both still work in this mode.")

+ }

+ return sess

// gitShortHead resolves -version's default: the working directory's short

// git SHA. Fatal here (rather than a returned error) matches every other

// "this environment is wrong" refusal in this binary — there is no sane

@@ -161,6 +189,39 @@ func resolveDeployChannel(recs []apiInstanceRecord, hostFlag string) (channel st

return "", nil, errors.New(b.String())

}

+// filterChannelHosts computes the hosts an EXPLICIT -channel deploy should

+// watch: instance records whose Channel matches, further scoped to hostFlag

+// if given. hostFlag is validated against the instance list up front —

+// independent of channel — so a typo'd -host is its own error before ship

+// and promote ever run, rather than silently producing zero hosts and

+// surfacing as "no instance follows <channel>" once the writes have already

+// landed (task-9 review, finding 3). This mirrors resolveDeployChannel's own

+// host-existence check on the non-explicit-channel branch: an existing host

+// that simply doesn't follow the requested channel is not an error here

+// (the caller may be deploying a channel that host has never served before)

+// — only a host absent from the instance list entirely is.

+func filterChannelHosts(recs []apiInstanceRecord, channel, hostFlag string) ([]string, error) {

+ if hostFlag != "" {

+ found := false

+ for _, r := range recs {

+ if r.Host == hostFlag {

+ found = true

+ break

+ }

+ if !found {

+ return nil, fmt.Errorf("no instance with host %q", hostFlag)

+ }

+ var hosts []string

+ for _, r := range recs {

+ if r.Channel == channel && (hostFlag == "" || r.Host == hostFlag) {

+ hosts = append(hosts, r.Host)

+ }

+ return hosts, nil

// watchTarget is one instance to watch: Host is what gets printed (the

// public hostname the operator will actually visit) and Origin is what gets

// dialed (https://<host> in production; a test's httptest.Server URL in

diff --git a/deploy_test.go b/deploy_test.go

index 201fa27ae..12d291e45 100644

--- a/deploy_test.go

+++ b/deploy_test.go

@@ -32,6 +32,40 @@ func TestResolveDeployChannel(t *testing.T) {

}

+// TestFilterChannelHosts pins the -channel+-host validation fix (task-9

+// review, finding 3): a -host that names no instance at all must error —

+// not silently produce zero hosts, which would let deploy ship and promote

+// against a typo'd host and only then shrug "no instance follows <channel>",

+// blaming the channel and never naming the host. A host that DOES exist but

+// simply isn't on the requested channel is not an error (the caller may be

+// pointing a channel at a host for the first time).

+func TestFilterChannelHosts(t *testing.T) {

+ recs := []apiInstanceRecord{

+ {Host: "a.test", Channel: "edge"},

+ {Host: "b.test", Channel: "beta"},

+ }

+ hosts, err := filterChannelHosts(recs, "edge", "")

+ if err != nil || len(hosts) != 1 || hosts[0] != "a.test" {

+ t.Fatalf("unscoped: %v %v", hosts, err)

+ }

+ hosts, err = filterChannelHosts(recs, "edge", "a.test")

+ if err != nil || len(hosts) != 1 || hosts[0] != "a.test" {

+ t.Fatalf("host-scoped, matching channel: %v %v", hosts, err)

+ }

+ hosts, err = filterChannelHosts(recs, "edge", "b.test")

+ if err != nil {

+ t.Fatalf("an existing host on a different channel must not error: %v", err)

+ }

+ if len(hosts) != 0 {

+ t.Fatalf("b.test follows beta, not edge — must not appear: %v", hosts)

+ }

+ if _, err := filterChannelHosts(recs, "edge", "typo.test"); err == nil {

+ t.Fatal("a -host naming no instance at all must error")

+ } else if !strings.Contains(err.Error(), "typo.test") {

+ t.Fatalf("error must name the offending host: %v", err)

+ }

func TestWatchVersionSucceedsWhenHeaderMatches(t *testing.T) {

var mu sync.Mutex

ver := "old1234"