Skip to content

fix(deploy,nginx,test): rolling gate limit and bounded up, $ in proxy targets, next-step deadline assert (D-0469, D-1139, D-0492, D-0237) - #961

Open
acamarata wants to merge 5 commits into
mainfrom
dw2-cli-2
Open

acamarata wants to merge 5 commits into
mainfrom
dw2-cli-2

Conversation

@acamarata

Copy link
Copy Markdown
Collaborator

Per row

D-0492 (fixed). TestNextStep no longer bounds wall-clock time (a Windows runner took 9.5s). It asserts the deadline path: the slow case must end with ctx.Err() == context.DeadlineExceeded and the result doctor; every other case must finish with ctx.Err() == nil.

D-0469 (fixed, three of four items plus docs).

  • gateKind prints the limit HealthGate really applies, through a new docker.GateLimit(spec) that HealthGate now uses too, so the two cannot drift (healthy, max 105s, capped at 600s).
  • up -d --no-deps in the rolling path is bounded at 15 minutes (rollingUpTimeout). A hang ends in a named error and docker is stopped.
  • An interrupted up (ctx cancelled, or docker exiting 130 before nself's handler runs) is reported as an interrupt, not a state-read failure, and the error says Service <svc> may be stopped: run 'nself restart <svc>' to bring it back.
  • Deviation from the row text: it suggests nself start <svc>, but nself start is cobra.NoArgs. nself restart [SERVICES...] is the command that takes services, so the message uses that.
  • .github/wiki/cmd-deploy.md updated (limit table, new paragraph). cli#629 and cli#657 are both merged.
  • The new helper lives in cmd/commands/deploy_rolling_up.go to keep deploy_strategies.go under 300 lines.

D-1139 (fixed). The generated conf is set $up_x <target>; proxy_pass $up_x;, so nginx expands any $name in a target while routes.json recorded the literal text. validateTarget now refuses $ (error says to write %24 for a literal dollar sign). TestValidateTarget gained cases; they fail on the old build.go and pass now. The location in the row (route.go:99) is stale; the validator is in routemodel/build.go. Nginx-Generation.md updated. The review report named in the row was not available to me, so refusing $ follows the validator's existing "refuse what the model cannot represent" rule; say so if the report wanted something else.

D-0237 (already fixed in the part the row describes; wrapper kept). The ResetFlags mutation was removed in #660 (env_target_test.go builds fresh command copies, and no test calls ResetFlags any more). I tried dropping the fresh-process wrapper in TestRegistryGolden: the golden then fails in a full in-process run (admin group account vs committed plugins). Bisecting the test order, it first breaks once TestCanonAccountResolution is in the run. So the wrapper is still needed. This PR only corrects the stale comment that blamed env_target_test.go. Dropping the wrapper needs a separate fix for that leak.

D-0182 (not done, product decision). config env explain and config explain are two commands over the shared config.ExplainKey (different run functions, different output shapes). The row's resolution is "revisit at v1.6.0 planning", which is a command-surface decision, not a code fix.

Commands run

  • go vet ./cmd/... ./internal/docker/ ./internal/nginx/... clean; gofmt -l clean.
  • go test ./cmd/... ./internal/docker/... ./internal/nginx/... ./internal/deploy/... -count=1: all ok.
  • TestRollingUp, TestGateKindPrintsTheRealLimit, TestGateLimit, TestNextStep, TestValidateTarget pass; the D-1139 cases were checked to fail against the old build.go.
  • golangci-lint run could not be used here: it fails with typecheck errors on the standard library (go 1.27.2 toolchain), before reaching repo code.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Rebased onto current origin/main (6 commits behind, now a8250fa). Nothing needed fixing beyond the rebase: go build, go vet, gofmt and the full go test ./... pass locally on the rebased head, including the repoqa ratchets (funnel allowlists, file budget, panic baseline) and the nginx call-site list. No ratchet was changed. The gate output given was only a log-glob error from the gate host with no test failure in it, so the failure looks like staleness against main.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Rebased onto current origin/main. The only failure on the rebased head was TestRollingUp/a_cancelled_context_prints_the_recovery_line. It cancelled after a fixed 300ms, which on a loaded host landed while postgres was still coming up (error read 'interrupted at service postgres'). The fake docker now drops an uphanging marker once web's up hangs, and the test cancels on that marker. cmd/commands and internal/... pass in full. The nginx call-site, docker funnel, file-budget and panic ratchets are untouched. The gate log path in the report was an unexpanded glob, so there was no log to read.

@acamarata

Copy link
Copy Markdown
Collaborator Author

Rebased onto current origin/main (clean, no conflicts) and force-pushed (0c28d73). With main's new ratchet sites in the tree, CGO_ENABLED=0 go test -mod=vendor -count=1 ./... and go vet -mod=vendor ./... are green locally; no ratchet edits were needed. The reported gate output was only a missing-log glob error from the gate host (zsh: no matches found), not a test failure, so this needs a re-run of nself-ci on the new head.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant