Fix --setup handling and nil-deref crash in service push#253
Open
l-hellmann wants to merge 3 commits into
Open
Fix --setup handling and nil-deref crash in service push#253l-hellmann wants to merge 3 commits into
l-hellmann wants to merge 3 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Type of Change is this?
Description (required)
Follow-up to #252. Fixes the two confirmed bugs documented there, plus a
related UX improvement uncovered by the first fix. Converts the two
previously-skipped reproductions into live regression tests.
1.
--setupflag is ignored when service name matches a yaml setupservicePush.goandserviceDeploy.gopreviously checkedgn.FindFirstonthe service name before consulting the
--setupflag. If a service namedbackendhad abackendsetup in its yaml, an explicit--setup showcase-backendwas silently overridden by the auto-match.Reported from a real pipeline running
zcli push backend --setup showcase-backend.Fix (52f69fa): invert the branches so an explicit flag always wins.
Precedence is now
--setupflag > auto-match by service name > interactiveselector (TTY) or hard error (non-TTY).
2. SIGSEGV nil-deref in push log-streaming callback
servicePush.go's log-streaming callback dereferencedapiProcess.AppVersion.IdandapiProcess.AppVersion.Buildon the firstpoll that reported
status=RUNNING. BothAppVersionandBuildarenullable pointers in
output.Process— when the API returnedRUNNINGbefore
AppVersionwas populated, the CLI nil-deref'd in the spinnergoroutine and crashed with SIGSEGV (Go's runtime can't recover panics from
goroutines you don't own).
Original stack trace, reproduced before the fix:
Fix (47918ac): nil-guard
apiProcess.AppVersionbefore the deref sites,plus defer the
Buildderef behind aBuild != nilcheck.3. Client-side validation of
--setupagainst the local yamlBefore the precedence fix, the auto-match could mask typos: a
--setup typoon a service whose name happened to match a yaml setup would silently fall
back to the auto-matched setup. After the precedence fix, an unknown
--setupvalue reaches the server-side yaml validation, which does rejectit: the API responds with
errorCode: zeropsYamlSetupNotFound, and theCLI's
validateZeropsYamlContentalready maps it througherrorsx.Convertto a user-facing message via
i18n.ErrorServiceNotFound->"Service [name] not found".That works, but the existing message has two warts:
"Service [...] not found"even though the badvalue is a setup, which is confusing.
Fix (74c24b1): new helper
validateSetupInYamlinservicePushDeployShared.gorejects unknown values before any API callwith a setup-specific message that also lists what's available:
Test impact
Two previously skipped tests are now live regression guards:
TestServicePushCommand_SetupFlagOverridesAutoMatch-- pins the precedence fixTestServicePushCommand_RunningProcessWithNullAppVersionNoCrash(renamed from
_BUGPROBE) -- pins the nil-deref fixOne existing test was rewritten to match the new contract:
TestServicePushCommand_SetupNotFoundInYaml_ForwardedToApi->TestServicePushCommand_SetupNotFoundInYamlRejectedLocally. Assertsnon-zero exit, error message names the bad value, lists available
setups, and confirms no upload/deploy API calls were made.
Test plan
make test-integration-- 28 PASS, 0 SKIP, 0 FAIL (was 26 PASS, 2 SKIP in Add integration test harness, push/deploy + TUI tests, document two production bugs #252).make test-- unit tests pass.make lint-- clean.Related issues & labels (optional)
Stacked on #252; base will retarget to
mainautomatically after #252 merges.