-
Notifications
You must be signed in to change notification settings - Fork 16
fix: report unknown release versions instead of a server null reference #696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
NickJosevski
wants to merge
5
commits into
main
Choose a base branch
from
nj/issue-294
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+187
−53
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
af509e5
fix: report unknown release versions instead of a server null reference
NickJosevski 2541136
fix: don't assert "not found" when the release lookup is ambiguous
NickJosevski cf2dc4c
refactor: drop the now-unreachable web-URL release lookup
NickJosevski 14ff2ee
fix: only a missing release aborts the deploy pre-flight
NickJosevski 4e9ec94
refactor: call selectors.FindRelease directly from GetReleaseID
NickJosevski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behavioral note: this pre-flight makes every automation-mode deploy depend on being able to GET the release (ReleaseView), which the old flow never required — the executions API only ever saw the version string. A CI service account scoped to deploy but not to read releases (or a transient 5xx on this GET) now aborts a deploy that previously succeeded, since non-404 errors are returned untouched. Probably an acceptable trade, but worth a conscious decision — an alternative is to treat only a definitive 404 as fatal and fall through to the POST on any other lookup failure.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actioned in 14ff2ee. The pre-flight now only aborts on
selectors.ReleaseNotFoundError(viaerrors.As); any other error leavesoptions.ReleaseIDempty and falls through to the POST, letting the server stay the authority. New automation-mode test covers a 403 from the release GET still reaching the deploy.One residual worth naming, because it is narrower than it looks. The fatal/non-fatal split is not really "was it a 404" — it is "did the response carry a body".
DoRawJsonRequestshort-circuits onresp.ContentLength == 0before it ever reads the status, so the status code is only available for responses that had a decodable JSON body:APIErrorbody →Confirmed: true→ fatal (correct)Confirmed: false→ fatal (correct, and the reason this case cannot be made non-fatal — it would put the NRE back)Content-Length: 0→ indistinguishable from the row above it → still fatal, reported as "could not resolve …"So the mitigation covers failures that come back with a body, and the bodyless non-404 is the leftover. Note a genuine proxy 502 usually has an HTML body, which fails to decode as
APIErrorand returns a decode error — also non-fatal. That leaves the gap at explicitly bodyless non-404 responses, which is a small set.The question that would close it properly: does Octopus
GET /api/{space}/projects/{id}/releases/{version}return a 404 with anAPIErrorbody for a version that does not exist? If it always does, the empty-body branch is only ever reached by non-404s and could safely be non-fatal, collapsing the residual entirely. This PR handles both shapes defensively but does not establish which one the server actually sends — worth confirming against a real instance before deciding.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked against a live instance (md.octopus.app, Octopus Server 2026.3.14820, Octopus Cloud). The residual is not reachable there —
Confirmed: trueis the real path.Raw HTTP for
GET /api/Spaces-1/projects/{id}/releases/{missing-version}:The 404 carries an
APIErrorbody, so it decodes on the error path and arrives asConfirmed: true.The framing detail matters more than the body, though. This endpoint never sends
Content-Length— HTTP/2 omits it, and forcing--http1.1givestransfer-encoding: chunked. Go setsresp.ContentLength = -1in both cases, soresp.ContentLength == 0inDoRawJsonRequestis false and the short-circuit is never taken at all, regardless of status code. Verified through the real SDK rather than inferred:So on this server the
Confirmed: falsebranch is unreachable, which also means the bodyless-403/502 case I worried about cannot arise from the server itself. It could still arise from something in front of it (a customer reverse proxy emittingContent-Length: 0), so the branch is worth keeping as a guard — but as a guard, not as an expected path. I would leave 14ff2ee as-is; it costs nothing and theerrors.Assplit is the right shape either way.