test: integration tests for the tier 1 release fixes - #703
NickJosevski wants to merge 24 commits into
Conversation
`--deployment-target "ABC,XYZ"` was sent to the server as a single target name because the flag is a pflag StringArray, while its legacy aliases (`--target`, `--specificMachines`) are StringSlice and already split on commas. Expand comma-separated values for the environment, tenant, tenant-tag and target flags on `release deploy` and `runbook run`, so the comma form matches the repeat-the-flag form. Values that can legitimately contain a comma (--variable, --skip, package/git-resource specs) are left alone. Fixes #556 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
`release create --no-prompt` sends the create request straight to the server without resolving package versions first. When a package has no version in its feed the server raises a null reference exception, which surfaces as "Octopus API error: Object reference not set to an instance of an object. []". On a 5xx failure the CLI now repeats the package version resolution the server does, and reports the packages, steps and feeds that have no version available. Where it can't identify a specific package, an unhandled server error now carries a hint about the likely causes. Fixes #426 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
`release deploy` passed --version straight to the executions API, which answers an unknown version with "Object reference not set to an instance of an object". Resolve the release before deploying so a version that doesn't exist is reported by name, and call out `latest` explicitly since it is not a supported alias. Refs #294 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…enant The executions API only matches channels, environments and tenants by name, so `release create`, `release deploy` and `runbook run` passed whatever the caller typed straight through and the server rejected IDs. `--project` already worked because the server accepts a project ID or name. Resolve those identifiers client side through the shared selectors package before handing them to the executor, preferring an ID match over a name match so it behaves the same way as `--project`. Fixes #250 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
| }) | ||
| if err != nil { | ||
| return err | ||
| return DiagnoseCreateReleaseFailure(octopus, options, err) |
There was a problem hiding this comment.
Nil pointer dereference on the post-create lookup failure path (pre-existing, but this function is being touched here and the new diagnosis flow makes create failures more visible): a few lines below at the options.Response handling, when octopus.Releases.GetByID(options.Response.ReleaseID) fails, the error branch still dereferences the nil result:
newlyCreatedRelease, lookupErr := octopus.Releases.GetByID(options.Response.ReleaseID)
if lookupErr != nil {
cmd.PrintErrf("Warning: cannot fetch release details: %v\n", lookupErr)
printReleaseVersion(options.Response.ReleaseVersion, newlyCreatedRelease.Assembled, newlyCreatedRelease.ReleaseNotes, nil)ReleaseService.GetByID returns nil, err on failure, so a transient server error right after a successful create panics the CLI instead of printing the warning.
There was a problem hiding this comment.
The finding holds on this branch. pkg/cmd/release/create/create.go:366-369 still reads newlyCreatedRelease.Assembled and .ReleaseNotes on the lookupErr != nil branch, and ReleaseService.GetByID returns nil, err, so a transient failure right after a successful create panics.
Not fixing it here, though: #722 ("fix: don't dereference a nil release when the post-create lookup fails") exists for exactly these lines, and its diff is the fix — it also covers the lookupErr == nil && newlyCreatedRelease == nil case, prints time.Time{}, "" instead of reading off the nil, and has a release creation warns, rather than panicking, when the post-create lookup fails unit test. A second edit to the same four lines on this branch would just be a conflict for whichever lands second.
The dereference is pre-existing on main and reaches this branch unchanged — nothing in the tier-1 merges touched it — so this PR doesn't have to carry it. Happy to be told otherwise if you'd rather it ride along with the integration tests, since the diagnosis flow does make create failures more visible.
|
|
||
| // diagnosis is best-effort; if any part of it fails we must not mask the original failure | ||
| if octopus != nil && options != nil { | ||
| if missingPackages, findErr := findPackagesWithoutVersions(octopus, options); findErr == nil && len(missingPackages) > 0 { |
There was a problem hiding this comment.
The package diagnosis can misattribute an unrelated 5xx and hide the real cause. This branch runs for any 5xx, not just the null-reference case, and MissingPackageVersionsError.Error() does not include the original server message (it is only reachable via Unwrap). If the server 500s for an unrelated reason (timeout, genuine server bug) while the project happens to contain a package with no version in its feed — or the CLI's re-derived baseline disagrees with the server (e.g. a --package override the CLI silently failed to parse but the server accepted) — the user is told to push packages instead of seeing the actual failure.
Consider gating the missing-package diagnosis on strings.Contains(apiError.ErrorMessage, serverNullReferenceMessage) as well, and/or including the wrapped cause text in the error output.
There was a problem hiding this comment.
Half of this is already done on this branch, and the other half was tried and deliberately reverted — both on nj/issue-426 (#695), which owns the code, so nothing changed here.
The wrapped cause is now reported. MissingPackageVersionsError.Error() appends the server reported: <cause> unless the cause text is the null-reference message, which says nothing the diagnosis lines don't already say better (pkg/packages/packages.go:241-248, commit c12edf9). So the "told to push packages instead of seeing the actual failure" outcome no longer happens silently: the real 5xx message is in the output alongside the diagnosis.
The gate on serverNullReferenceMessage did land (ea972e2) and was then taken back out, with the reason left in the doc comment on DiagnoseCreateReleaseFailure: the message for this failure varies by server version — current servers answer with "no viable release plans" rather than a null reference — so gating on that string loses the diagnosis on the servers people actually run. What survives of the gate is the fallback: if the diagnosis finds nothing but the message is the null reference, the error is annotated with "the server failed with an unhandled error; this usually means it could not resolve the packages, channel or git reference".
Live check on this branch, which settles the version-variance argument. Ran TestReleaseCreateMissingPackageVersion against a local Octopus instance with the CLI's stderr dumped. Full output:
cannot create release; no version could be found for the following packages:
- 'package-9baebdb7-...' in step 'step-9baebdb7-...' (feed 'Octopus Server (built-in)')
push the package(s) to the feed, or supply a version with --package or --package-version
the server reported: Octopus API error: There are no viable release plans in any channels using the provided arguments. The following release plans were considered:
Channel: 'Default' (this is the default channel)
# Name Version Source Version rules
1 step-9baebdb7-... ERROR Cannot resolve Allow any version
So on this server the 500 carries "no viable release plans", not "Object reference not set" — a strings.Contains(apiError.ErrorMessage, serverNullReferenceMessage) gate would have suppressed the diagnosis entirely here. And the wrapped cause is visibly in the output, which is the part that addresses "hide the real cause".
So: real risk, now bounded rather than eliminated. If you want it narrower, the shape I'd suggest to #695 is a gate on the union of the known opaque messages (null reference, "no viable release plans") rather than the null reference alone — but that's a decision for that PR.
| // `--flag A --flag B`. Whitespace around each entry is trimmed and blank entries are dropped. | ||
| // Only apply this to flags whose values cannot legitimately contain a comma; notably NOT to | ||
| // --variable, --skip or the package/git-resource specs. | ||
| func ExpandCommaSeparated(values []string) []string { |
There was a problem hiding this comment.
Regression for names that legitimately contain commas, with no escape hatch. Octopus allows commas in environment/tenant/machine names. Before this change --environment "Dev, East" (one env named Dev, East) worked; now it is unconditionally split into Dev + East and the deploy fails with "cannot find an environment...". The doc comment acknowledges the constraint but there is no way for a user to opt out (quoting doesn't help — the split happens after shell parsing).
The old octo CLI had the same splitting behaviour, so this may be an accepted trade-off — but worth an explicit decision and a mention in the flag help/changelog, since previously-working invocations now break silently.
There was a problem hiding this comment.
There is an escape hatch now, and the flag help documents it — both from nj/issue-556 (#692), which owns this code.
ExpandCommaSeparated splits only on unescaped commas (splitOnUnescapedCommas, pkg/executionscommon/executionscommon.go:306-355, commit b77c149), so --environment 'Dev\, East' yields the single value Dev, East. A backslash anywhere else is preserved verbatim, so DOMAIN\host target names are unaffected. Covered by TestExpandCommaSeparated in pkg/executionscommon/executionscommon_test.go.
Every affected flag says so in its help text, e.g.
--environment Deploy to this environment (can be specified multiple times, or as a
comma-separated list; escape a comma inside a value as '\,')
and EscapeCommas re-escapes values on the way back out so the printed automation command round-trips.
The other half of your concern is now an error rather than silent: a blank component ("A,,B", a , from an empty variable substitution) fails with --environment has a blank value; check for an empty variable or a stray comma in "A,,B". Use '\,' to include a comma in a value (c64ab93), which also names the escape at the point of failure.
Changelog: d25b40c/b77c149/c64ab93 are fix: commits on #692 and CHANGELOG.md is release-please-generated, so there's nothing to hand-edit — the note comes from those subjects when the release is cut.
Residual, honestly: the split is still the default and the escape is opt-in, so a previously-working --environment "Dev, East" does break on upgrade until the user adds the backslash. That's the octo compatibility trade you describe. Flipping the default is a decision for #692, not this branch.
|
|
||
| // FindTenant looks a tenant up by either its ID or its name. | ||
| func FindTenant(octopus *octopusApiClient.Client, tenantIdentifier string) (*tenants.Tenant, error) { | ||
| tenant, err := octopus.Tenants.GetByIdentifier(tenantIdentifier) |
There was a problem hiding this comment.
False "cannot find a tenant" for tenants beyond the first page of a partial-name search. The SDK's Tenants.GetByIdentifier name fallback (GetByName) issues Get(TenantsQuery{PartialName: name}) and scans only the first page of results for an exact match — it never pages. On a space with many tenants whose names share a common substring (e.g. dozens of "Store ..." tenants), a tenant whose exact name lands beyond page 1 returns ErrItemNotFound, and this new resolution step fails a deploy/runbook run that previously worked (the raw name used to be passed straight to the executions API, which matched it fine).
Also note GetByIdentifier uses a direct type assertion on the GetByID error, so a non-APIError failure (network blip) silently falls through to the name lookup rather than being reported.
There was a problem hiding this comment.
The pagination half is fixed on this branch — 93ab1c6 on nj/issue-250 (#702), which owns this file. FindTenant deliberately does not use Tenants.GetByIdentifier; the doc comment now says why, in about the same words as your comment. It does GetByID, then falls back to a PartialName query that pages via resultPage.GetNextPage until it finds an exact EqualFold match or runs out of pages (pkg/question/selectors/tenants.go:20-52). Covered by TestFindTenants/finds an exact name match beyond the first page of the partial name search, which serves a first page that doesn't contain the match.
The second half is only partly addressed, and you're right to flag it. The GetByID error branch is:
if errors.As(err, &apiError) && apiError.StatusCode != 404 {
return nil, err
}so a non-APIError failure (a network blip) does not return — it falls through to the name lookup, same as before. In practice the same blip usually fails the name query too and that error is returned, so it isn't silent. The residual is the narrow interleaving: blip on the ID call, name query then succeeds and matches nothing, and the user gets cannot find a tenant with the ID or name of 'x' instead of the transport error.
Worth noting the fall-through is also load-bearing: a plain name like Coke produces a 404 from GET /tenants/Coke and must fall through, so the branch can't simply return every error. Tightening it means distinguishing "this looked like an ID and the server said no" from "the call didn't complete" — which is the same conversation as the round-trip comment below (only do the ID lookup when the identifier matches ^Tenants-\d+$), and that would make both problems go away at once. #702's call, not this branch's.
| // resolveEnvironmentNames maps environment names or IDs onto canonical environment names, because | ||
| // the executions API only matches environments by name. Ephemeral environments aren't part of the | ||
| // regular environment list, so they're looked up separately when the regular lookup comes up empty. | ||
| func resolveEnvironmentNames(octopus *octopusApiClient.Client, space *spaces.Space, environmentIdentifiers []string) ([]string, error) { |
There was a problem hiding this comment.
A mixed list of regular + ephemeral environment identifiers can never resolve. selectors.FindEnvironments errors on the first identifier that isn't a regular environment, and the fallback findEphemeralEnvironments errors on the first identifier that isn't ephemeral — so --environment regularEnv,ephemeralEnv now fails client-side with "cannot find an environment ..." even though before this change both names were passed through verbatim for the server to judge. Probably invalid server-side anyway (one channel type per deployment), but if so the current error message points at the wrong thing: it claims the regular env doesn't exist when it does. Resolving each identifier individually (regular-then-ephemeral per item) would handle both this and give a precise error.
There was a problem hiding this comment.
Fixed, and in the shape you suggested — 3d8333b on nj/issue-250 (#702), which owns this code. The function the comment is attached to (resolveEnvironmentNames, local to deploy.go) is gone; it is now selectors.ResolveEnvironmentNames (pkg/question/selectors/environments.go:82-124) and it resolves one identifier at a time: regular list first, then the ephemeral list for any identifier the regular list doesn't have. So --environment regularEnv,ephemeralEnv resolves both, and a genuine miss names the identifier that actually went missing instead of blaming the first one in the list.
Two details worth having on the record:
- the ephemeral list is fetched once and lazily, only when some identifier misses the regular list, so the common all-regular case doesn't pay for the extra call. If that endpoint doesn't exist on the server, the error is still reported as "cannot find an environment with the ID or name of 'x'" — the identifier genuinely isn't a regular environment either way.
deployandrunbook runnow call the same function, so the ephemeral fallback isn't duplicated between them any more.
Unit coverage in pkg/question/selectors/find_test.go: TestResolveEnvironmentNames/resolves a mix of regular and ephemeral environments, /doesn't look at ephemeral environments when everything resolves, and /names the environment that is actually missing.
Nothing changed on this branch for it; it arrived with the merge of nj/issue-250.
| idLookup := make(map[string]*environments.Environment, len(allEnvs)) | ||
| nameLookup := make(map[string]*environments.Environment, len(allEnvs)) | ||
| for _, env := range allEnvs { | ||
| idLookup[strings.ToLower(env.GetID())] = env |
There was a problem hiding this comment.
Silent precedence flip for existing callers. The old executionscommon.FindEnvironments checked the name lookup before the ID lookup; this new implementation checks ID first. Since executionscommon.FindEnvironments is now an alias to this, the change reaches all its existing callers (tenant connect, five target ... create commands, runbook, deploy): in a space where an environment is named the same as another environment's ID, those commands now resolve to a different environment than before. The tests show this is deliberate ("consistent with how projects and tenants resolve") — flagging it because it's an observable behaviour change to commands this PR doesn't otherwise touch, and may deserve a changelog note.
There was a problem hiding this comment.
Confirmed, the flip is real and it does reach commands this PR doesn't otherwise touch. origin/main's executionscommon.FindEnvironments checks nameLookup and only falls back to idLookup; the new selectors.FindEnvironments goes through identifierLookup.find, which is ID-then-name. executionscommon.FindEnvironments is now a one-line alias to it, so the new order applies to tenant connect, the five target ... create commands, runbook run's AskQuestions path and deploy's interactive path — not just the automation paths #702 set out to fix.
The blast radius is narrow but not empty: it only differs when one environment is named exactly another environment's ID (Environments-12), which is legal since names are free text. Nothing in the repo depended on the old order — all unit tests pass with ID-first — so this is an unobservable change except in that collision.
While we're enumerating observable changes to those callers, the error text also changed: cannot find environment environments-12 (lowercased identifier, because the old code lowercased before formatting) is now cannot find an environment with the ID or name of 'Environments-12'. That one is strictly better and I'd leave it.
Two things I'd like a decision on, since both are cheap and they point opposite ways:
- Keep ID-first everywhere for consistency with projects/tenants/channels, and describe the flip in the commit subject on fix: accept IDs as well as names for --channel, --environment and --tenant #702 so release-please renders it (CHANGELOG.md is generated, so a commit note is the only lever)? Or
- Keep ID-first for the new executions-API resolution and leave
executionscommon.FindEnvironmentsname-first for its existing callers — i.e. don't make it an alias — sotenant connectandtarget ... createare bit-for-bit unchanged?
I'd lean (1): one rule is easier to explain than two, and the collision that distinguishes them is pathological. But it's your call, and it belongs on #702 rather than here — I've made no change on this branch.
|
|
||
| // the executions API only matches tenants by name, so resolve any IDs we were given | ||
| if len(options.Tenants) > 0 { | ||
| selectedTenants, err := selectors.FindTenants(octopus, options.Tenants) |
There was a problem hiding this comment.
Every automation-mode invocation now pays extra HTTP round trips even when plain names were given. For each tenant supplied by name this is two requests (a guaranteed 404 on GET /tenants/<name>, then the partial-name search), plus GET /environments/all, plus the release lookup — sequentially, on every CI deploy. The same pattern is in runbook run (and there the environment/tenant resolution also runs in interactive mode, where AskQuestions resolves environments again — duplicate /environments/all calls).
A cheap win: only hit the ID lookup when the identifier actually looks like an ID (^Tenants-\d+$ / ^Environments-\d+$), or resolve tenants with a single query instead of per-tenant round trips. Related: selectors.FindEnvironment (singular) previously used a paged partial-name server query and now loads the entire space's environment list to find one environment.
There was a problem hiding this comment.
The counts, as the code stands on this branch, so the decision is on real numbers:
- tenants: 2 requests each, and one of them is a guaranteed 404 for plain names.
FindTenantalways triesGET /tenants/<identifier>before thepartialNamesearch.--tenant a,b,cby name is 6 requests, 3 of them 404s. This is the one that scales with input. - environments: 1 request total, not per identifier.
FindEnvironments/ResolveEnvironmentNamescallEnvironments.GetAll()once and match client-side, so--environment a,b,cis oneGET /environments/all. Worth correcting:FindEnvironment(singular) did get worse — it used to be a pagedpartialNamequery that could stop at the first page, now it loads the whole space's environment list for one lookup. On a space with a lot of environments that's a bigger response, but still one round trip. - release: 1 request, and on the default output format it is a net saving.
origin/mainlooked the release up after the deploy to print the web link, and did it asFindProject+GetReleaseInProject— two requests. Both are gone (cf2dc4c), so table output is now one request cheaper. It's a true +1 only with--output-format=json|basic, where the link was never printed and so no lookup happened.
Net for a typical CI release deploy --project P --version V --environment dev: +1 (environments/all) on table output, +2 on json/basic. With --tenant by name: 2 more per tenant on top.
The duplicate you predicted in interactive runbook run is real. run.go:263 resolves environments unconditionally (Environments.GetAll), and then run.go:861/875/1084/1098 call executionscommon.FindEnvironments again from the Q&A path, each of which is another GET /environments/all — so an interactive runbook run with --environment given fetches the full environment list at least twice.
The ^Tenants-\d+$ guard you suggest is the right first move and it's cheap: it removes the 404 per named tenant, and as a side effect removes the "network blip falls through to the name lookup" residual on the other thread, because the ID call would only be made when the identifier actually is an ID. The environment duplication wants the other fix — resolve once and thread the result through, which is the altitude point on the thread below.
What I'd like decided: is any of this in scope for the tier-1 fixes, or does it go in the follow-up with the altitude change? My read is that the tenant guard is small enough to land on #702 now (roughly a regexp plus a branch, with a unit test that asserts no GET /tenants/<name> is issued for a plain name), and the environment de-duplication needs the executor-layer refactor and should wait. If you agree I'll raise it on #702 rather than here.
| } | ||
|
|
||
| // the executions API only matches environments by name, so resolve any IDs we were given | ||
| if len(options.Environments) > 0 { |
There was a problem hiding this comment.
Altitude: the ID-to-name resolution is scattered per command, at differing depths. release create resolves the channel only in the automation branch; release deploy resolves tenants before both modes but environments only in the automation branch; runbook run resolves both unconditionally. The invariant they all enforce ("the executions API only matches by name") belongs to the layer that builds the executions-API commands (pkg/executor/release.go / the runbook executor), where one implementation would cover all three commands, both modes, and any future execution flag — instead of a pattern that has to be remembered (and is already applied inconsistently) in each command. Fine to land as-is for the tier-1 fixes, but worth a follow-up.
There was a problem hiding this comment.
Agreed, and the inconsistency is still exactly as you describe on this branch — I checked each one:
release create: channel resolved only in the automation branch (create.go:314-321).release deploy: tenants resolved before the mode split (deploy.go:274-281, so both modes), environments only in the automation branch (deploy.go:388). Interactive mode relies on AskQuestions resolving them separately.runbook run: both resolved unconditionally, before the mode split (run.go:262-275).
Three commands, three different placements, and the interactive runbook run path pays for it — it resolves environments at run.go:263 and then AskQuestions calls executionscommon.FindEnvironments again at run.go:861/875 (and 1084/1098 on the by-tag path), so the full environment list is fetched at least twice.
pkg/executor/release.go is the right home: the invariant is a property of the executions-API command shape, not of any one command's flag parsing, and putting it there also fixes the duplication above for free (resolve once, at the point the command is built).
No change here — this PR is the integration tests for the tier-1 fixes and a refactor of the executor's inputs would put the fixes and their reshaping in one changeset. Do you want me to raise the follow-up issue, and if so should it carry the tenant ^Tenants-\d+$ guard from the round-trip thread as well, or keep that separate on #702 where it's a two-line change?
The package diagnosis ran for any APIError with a 5xx status. On an unrelated server error that had the side effect of (a) replacing a real server message with MissingPackageVersionsError, whose Error() doesn't include the cause, and (b) firing ~6 extra requests at a server that is already failing. Require the null reference message before diagnosing, which is the only failure this code knows how to explain. The fallback hint no longer needs its own check, since reaching it now implies the message matched. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Two ways the replay could diverge from what the server actually did: - With --ignore-channel-rules the server resolves package versions without applying the channel's version rules, but the replay always applied them. A package with versions in its feed, none satisfying the rules, would be reported as "no version could be found", misdiagnosing the real failure. Build the baseline without the rule filter in that case. - --channel reaches the server as ChannelIDOrName, but the lookup matched on name only, so passing a channel ID silently dropped the diagnosis to the generic hint. Match on either. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The SDK's DoRawJsonRequest short-circuits on `resp.ContentLength == 0` and returns `(resp, nil)` for any status code, so DoRequest hands back a zero-valued Release with a nil error. A 404 with no body lands there, but so does a 403 with an empty body or a 502 from a proxy, and the status code is not recoverable at this layer. Reporting all of those as "cannot find a release with version X" is misleading during an outage or a permissions failure. Introduce selectors.ReleaseNotFoundError, which records whether the server confirmed the answer with a 404 carrying an APIError body, and hedge the wording when it did not. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
`Tenants.GetByIdentifier`'s name fallback (`GetByName`) issues a single `tenants?partialName=<name>` query and scans only the first page of the result. `partialName` is a contains filter, so an exact name that sorts past a page's worth of other tenants containing the same substring - e.g. `--tenant Smith` in a space full of `... Smith` tenants - came back as `ErrItemNotFound` and failed the deploy, even though the same name worked before this branch, when it was passed through and matched server side. `selectors.FindTenant` now does the ID lookup itself and walks every page of the partial name search looking for an exact match, keeping the same ID-beats-name precedence. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Review feedback: the five-line expansion block at the top of deployRun was duplicated verbatim in runbookRun, so any new multi-value flag has to be added to two hand-maintained lists. ExpandCommaSeparatedFlags takes the flags themselves and expands them in place, leaving one call per command. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
… them
Review feedback: dropping blanks let an explicitly-provided flag expand to
nothing. Because pkg/executor/release.go routes on
`len(params.Tenants) > 0 || len(params.TenantTags) > 0`, `--tenant "$A,$B"`
with both variables unset expanded to nil and the CLI silently submitted an
*untenanted* deployment to the environment. Before this branch the literal ","
was sent as a tenant name and the server rejected it. The same class of change
applied to `--exclude-deployment-target "$X"` with $X empty, where the
exclusion list quietly became empty.
A blank component always means a caller-side substitution produced nothing, so
ExpandCommaSeparated now returns an error naming the flag and quoting the
offending value. This also covers the partial case ("$A,$B" with only $B
empty), which would otherwise have silently narrowed the deployment scope.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Review feedback: the split was unconditional, so a tenant/target/environment named e.g. "Foo, Inc" could no longer be passed through the primary flags at all. The sharper edge was the interactive echo — a value chosen from a picker is backfilled into resolvedFlags and flag.GenerateAutomationCmd emits it verbatim, so the printed "Automation Command" was not re-runnable: pasting it into CI would split "Foo, Inc" back into two names, erroring if they don't exist or deploying to the wrong tenants if they do. `\,` now means a literal comma. A backslash anywhere else is preserved verbatim, so names such as DOMAIN\host are unaffected. Interactive selections are escaped with executionscommon.EscapeCommas on the way into the automation command, so the echoed command round-trips. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
With the release resolved before the deploy, `options.ReleaseID` is always set on both paths that reach the link: AskQuestions in interactive mode, the pre-flight lookup in automation mode (the executor rejects the deploy unless both ProjectName and ReleaseVersion are set, which is exactly when the pre-flight runs). The FindProject + GetReleaseInProject fallback can only run when the pre-flight lookup already failed, where repeating it would fail too. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The pre-flight lookup is new to `release deploy`; before it, the automation path never read the release and the executions API only ever saw the version string. Failing the whole deploy on any lookup error would break a CI service account scoped to deploy but not to ReleaseView, and would turn a transient 5xx on that GET into an aborted deployment that previously succeeded. Fail only on a ReleaseNotFoundError, which is the case issue #294 is about. For anything else, carry on without the release ID and let the server remain the authority on permissions and availability. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
shared.FindRelease was left as a one-line passthrough, so remove it. Doing so also puts GetReleaseID's spaceID parameter to use — it was accepted and then ignored in favour of octopus.GetSpaceID(). Both callers already pass opts.Client.GetSpaceID(), so the resolved space is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…emeral fallback with runbook run The ephemeral fallback was all-or-nothing over the whole `--environment` list: a list mixing a regular and an ephemeral environment could never resolve, because the regular lookup errored on the ephemeral name and the ephemeral lookup then errored on the regular one, leaving the user with `cannot find an environment with the ID or name of '<ephemeral env>'` - blaming an environment that exists. It also fell back on *any* error from the regular lookup, including a transport failure. `selectors.ResolveEnvironmentNames` now resolves each identifier in turn against the regular environment list, consulting the ephemeral list only for identifiers that list doesn't have (fetched once, lazily). Single-type lists behave exactly as before; mixed lists resolve, and a genuine miss names the identifier that actually went missing. `runbook run` uses the same resolver, so an ephemeral environment name that used to be passed through to the server no longer fails client side. Also flips ephemeral name/ID indexing in `findEphemeralEnvironments` so an ID match wins a collision, matching the precedence everywhere else. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
ccb5646 to
97602a7
Compare
ea972e2 narrowed the diagnosis to failures carrying the server's null reference message, to stop an unrelated 5xx being reported as a package problem. That works, but it also switches the fix off on current servers: the #426 path there fails with "There are no viable release plans in any channels", not a null reference, so the message the server sends for this is version-dependent and can't be relied on as the trigger. Address the underlying complaint instead. MissingPackageVersionsError now prints what the server actually said, so a misattributed diagnosis costs the user a misleading paragraph rather than the real cause, which was previously reachable only via Unwrap and never printed (main.go prints err.Error() alone). With nothing hidden, the trigger widens back to any 5xx and keeps working across server versions. The null reference message itself is still suppressed from that output -- it says nothing the diagnosis doesn't say better -- so the integration test's guard against it resurfacing stays valid. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Covers behaviour that only a real server exercises: unknown release versions, packages with no version in their feed, channel and environment IDs on the executions API, and comma-separated deployment targets. Refs #294, #426, #250, #556 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
… fixes Each of the four fixes is green on its own branch, but merged they change each other's request sequences, and the unit tests that merged cleanly are the ones that break. Nothing here is a defect in an individual PR; it is ordinary merge fallout, recorded because whichever lands last will hit it. - #294 adds a release pre-flight lookup and #250 an environments/all lookup ahead of the deployment POST. The tests added by #556, and the --priority tests that arrived on main in #708, merged without conflict and went looking for the POST, finding a GET. For the tenanted comma test the result was a hang rather than a failure: MockHttpServer blocks waiting for a request the CLI no longer makes in that order. - #250's "specifying project, environment and tenant by ID" still expected the two post-deploy web-URL lookups that #294 drops; the pre-flight now supplies the release ID, so no request follows the POST. - #556's runbook comma test needed #250's environments/all lookup, which runbook run performs unconditionally. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
97602a7 to
54072b6
Compare
Restacked onto current
|
| // and we simply go without the release ID. | ||
| release, err := selectors.FindRelease(octopus, f.GetCurrentSpace().ID, project, options.ReleaseVersion) | ||
| var releaseNotFound *selectors.ReleaseNotFoundError | ||
| if errors.As(err, &releaseNotFound) { |
There was a problem hiding this comment.
Only stop the deployment for a confirmed missing release.
FindRelease also returns ReleaseNotFoundError{Confirmed: false} for an empty response, including an empty-body 403 or 502 (as its new comment explains). This check ignores Confirmed, so those lookup failures still abort a deployment that previously went straight to the executions API. I reproduced the full command path with an empty-body 403: it exits with could not resolve a release... before submitting the deployment. The existing permission test only covers a 403 with an API-error body, which takes the intended fallback.
Please require releaseNotFound.Confirmed before treating the preflight as fatal; otherwise leave the release ID unset and let the deployment endpoint decide. Add empty-body 403/502 cases alongside the existing permission test, while retaining the structured-404 rejection test.
There was a problem hiding this comment.
Actioned in 5890921. The pre-flight now requires releaseNotFound.Confirmed before it returns the error; an unconfirmed ReleaseNotFoundError leaves options.ReleaseID unset and the deployment goes to the executions API exactly as it did before this PR.
Tests, in TestDeployCreate_AutomationMode:
release deploy proceeds when the release lookup is forbidden with an empty body(403, no body)release deploy proceeds when the release lookup hits an empty-bodied gateway error(502, no body)release deploy proceeds when the release lookup is forbidden with an error body— your existing permission case, behaviour unchanged
All three assert the same thing (the POST /deployments/create/untenanted/v1 body, no release ID, no web link), so they now share one parametrised body, deploysDespiteFailedReleaseLookup, which takes a function that answers the release request. Otherwise it was a 40-line table entry duplicated three ways.
Both new cases genuinely bite: dropping && releaseNotFound.Confirmed and re-running fails exactly those two and nothing else in the table.
One existing case changed shape. release deploy explains that 'latest' is not a supported release version answered with RespondWithStatus(404, "NotFound", nil) — an empty body, which is now non-fatal, so that case would have started submitting a deployment instead of failing. A real server answers that request with a 404 carrying an APIError body (per the live check on 2026.3.14820 recorded on #696), so the mock now sends that body and the case still asserts a fatal, confirmed rejection; the message just loses the hedge:
cannot find a release with version 'latest' in project 'Fire Project'. 'latest' is not a supported alias, specify an exact version. ...
release deploy reports a release version that doesn't exist (structured 404) is untouched.
Residual, stated plainly: on a server where a bodyless 404 really is the answer for a missing version, --version latest no longer gets the alias hint from the pre-flight — the deployment is submitted and the server's own error surfaces instead. That is the trade this asks for, and the live evidence on #696 says the unconfirmed branch isn't reachable from the server itself, only from something sitting in front of it.
Note on where this lives: these lines reached this branch from nj/issue-294 (#696) via the merge. The finding was raised here so the fix is here; if #696 lands first, the same one-line condition needs the same edit there.
| t.Cleanup(func() { deleteAllReleasesInProject(t, apiClient, project) }) | ||
|
|
||
| t.Run("create accepts a channel ID", func(t *testing.T) { | ||
| _, stdErr, err := integration.RunCli(space1ID, "release", "create", "--project", project.Name, "--channel", fx.ProjectDefaultChannel.GetID(), "--version", "1.0.0") |
There was a problem hiding this comment.
Use a non-default channel to prove the requested channel ID is honoured.
This project has only its default channel, and the assertion expects that same channel. The test therefore also passes if the CLI drops --channel entirely and lets the server select the default. It detects the old ID-as-name error, but does not distinguish correct resolution from silently ignoring the selection.
Create a second, non-default channel in the fixture, pass its ID here, and assert that the resulting release belongs to it. That makes the integration test verify the destination as well as successful release creation.
There was a problem hiding this comment.
Actioned in 89cca7d. The project now gets a second channel (createSecondaryChannel, no lifecycle of its own so it inherits the project's, no version rules so any version is valid in it), the case passes that channel's ID, and it asserts the release landed there — plus assert.NotEqual against the default channel, so the "server picked the default" outcome is named as the thing being excluded.
Verified against a live instance (local Octopus on Spaces-1, admin key):
- as written:
--- PASS: TestReleaseCreateAndDeployByID/create_accepts_a_channel_ID - mutation check: patched
release createto resolve--channeland then discard it (options.ChannelName = ""afterFindChannel), rebuilt the CLI, re-ran —Not equal: expected "Channels-353", actual "Channels-352", i.e. the secondary channel vs the project default.
So the failure mode you describe is now detected, and the previous assertion would have passed through it.
Two deliberate choices:
- the channel is created in the test, not in
CreateCommonProject. fixtures.go asks for shared fixture data that doesn't need putting back, and a second channel changes what the server picks for the other tests that callrelease createwith no--channel(TestReleaseCreateBasics,TestReleaseCreateMissingPackageVersion). - its
t.Cleanupis registered beforedeleteAllReleasesInProject, so cleanups run releases-first and the channel delete isn't refused for having a release in it.
What I could and couldn't run: these tests are gated on OCTOPUS_TEST_URL/OCTOPUS_TEST_APIKEY (no build tag — GetApiClient calls os.Exit(999) when they're unset), so go test ./... can't reach them. I ran them against a local server and all six pass — TestReleaseCreateBasics, TestReleaseListAndDelete, TestReleaseDeployUnknownVersion, TestReleaseCreateMissingPackageVersion, TestReleaseCreateAndDeployByID, TestReleaseDeployCommaSeparatedTargets — but only individually or in small groups. A -run TestRelease run of the whole file starts answering Octopus API error: Rate limit exceeded partway through, which also takes out the fixture teardown, so I can't tell you the file is green end to end in one pass. On my local instance that looks like the box's limiter rather than the tests, but if the nightly job ever fails this way, that's the shape of it.
FindRelease reports an empty response body as an unconfirmed ReleaseNotFoundError, because the SDK decodes a bodyless response as a zero-valued release with no error whatever its status code. Aborting on that turns a bodyless 403, or a proxy's 502, into a failed deployment that previously went straight to the executions API. Require Confirmed before treating the pre-flight as fatal; otherwise leave the release ID unset and let the deployment endpoint decide. The 'latest' alias test now answers with the APIError body a real server sends for a missing version (checked on 2026.3.14820), so it still exercises the confirmed rejection path. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The channel-ID case asserted that the release landed in the project's only channel, which is also where the server puts a release when no channel is requested - so it passed whether or not --channel reached the server. Add a non-default channel and assert the release lands there. Verified against a live instance (localhost:8065, Spaces-1): the test passes as written, and fails with "expected Channels-353, actual Channels-352" when release create is patched to drop the resolved channel, which the old assertion did not detect. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
1 participant
Adds end-to-end integration tests for the four "Tier 1" fixes — the ones whose correctness depends on how the real Octopus Server behaves, which
testutil.MockHttpServercannot validate by construction.Refs #294, #426, #250, #556.
Why this branch stacks the four fixes
The tests assert post-fix behaviour, so they need the fixes present to pass. This branch merges the four feature branches and adds the tests on top:
nj/issue-294nj/issue-426nj/issue-250nj/issue-556This branch is not for merging as-is. It exists to prove the four compose and to carry the new tests. Once the four land on
main, the last commit here rebases ontomainon its own.Tests added
All in
test/integration/release_test.go, following the existing harness (integration.RunCli,CreateCommonProject,t.Cleanupteardown).TestReleaseDeployUnknownVersion— release deploy returned error when using latest as input param for version #294. Asserts the API premise directly (an unknown version yields no usable release), then thatrelease deploynames the version it could not find, and thatlatestis explained rather than passed through.TestReleaseCreateMissingPackageVersion— Unhelpful output when attempting to create a release with a package that doesn't exist #426. Project with a package step whose package has no version in the built-in feed; asserts the failure names the package and the step.TestReleaseCreateAndDeployByID— Some arguments do not accept the ID instead of the name #250.release create --channel <Channels-N>lands on that channel;release deploy --environment <Environments-N>produces a deployment in that environment.TestReleaseDeployCommaSeparatedTargets— Support comma-delimited values on octopus release deploy --deployment-target command #556. Two cloud-region targets,--deployment-target "A,B", asserts the resulting deployment'sSpecificMachineIdscontains both.Verification
Run against a real Octopus Server (local dev instance, server
main):mainwith the same test file: all 4 fail. They are genuine regression tests, not tests that pass either way.go build ./...clean;go test ./pkg/...green (63 packages).Finding: the null-reference symptom no longer reproduces
#294 and #426 both describe
Octopus API error: Object reference not set to an instance of an object. []. On a current server that is not what happens:Octopus API error: Release 9.9.9 for project <name> was not found. []Octopus API error: There are no viable release plans in any channels using the provided arguments... Cannot resolveThe server-side defect appears to have been fixed since those issues were filed (2022.3 and 2024.4 respectively). Both CLI fixes still improve the message materially, and older servers still exhibit the original behaviour — but the premise that the server null-refs is no longer true on current versions. Two consequences:
assert.NotContains(..., "Object reference not set")lines in these tests are not load-bearing on a current server. They are kept as regression guards for older ones; the positive assertions are what carry the tests.Finding: the four fixes do not compose without test changes
Each of the four is green on its own branch, but merged they break each other's unit tests — invisible on the individual branches by construction. Fixed in the first commit here:
pkg/cmd/release/deploy/deploy_test.go, 8 conflict hunks: release deploy returned error when using latest as input param for version #294 adds a release pre-flight lookup and Some arguments do not accept the ID instead of the name #250 adds an environment lookup to the same request sequences. Resolved by expecting both, release first, matching the merged resolution order indeployRun.MockHttpServerdeadlocked waiting for a request that never came — the full unit suite hung rather than failed.release deploy specifying project, environment and tenant by IDstill expected the two post-deploy web-URL lookups that release deploy returned error when using latest as input param for version #294 removes.environments/alllookup.None of these are defects in the individual PRs; they are ordinary merge fallout. Flagging them because whichever of the four merges last will hit exactly this, and the failure mode for two of them is a hang, not a red test.
Notes
test/integrationhas no build tag andGetApiClientcallsos.Exit(999)whenOCTOPUS_TEST_URL/OCTOPUS_TEST_APIKEYare unset, so a barego test ./...from the repo root hard-exits. Run the suite fromtest/integration.allowDeploymentsTorestores the fixture lifecycle's phases on cleanup, otherwise the environment cannot be deleted.🤖 Generated with Claude Code