-
Notifications
You must be signed in to change notification settings - Fork 16
test: integration tests for the tier 1 release fixes #703
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
base: main
Are you sure you want to change the base?
Changes from all commits
954e06e
e417d72
2367871
9201499
8159197
8278287
b6dc414
f1f1607
a44de49
fcde44c
b4ec69a
e18d9e5
8907e0e
aee96fe
88aba55
4ba0de5
de14d35
266fe8c
cf47112
3174a9a
788222a
d33f2b8
dab4e16
d7ecd14
2517c55
2761779
8ddd0af
f020d53
0bce93b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import ( | |
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "os" | ||
| "strings" | ||
| "time" | ||
|
|
@@ -28,6 +29,7 @@ import ( | |
| "github.com/OctopusDeploy/cli/pkg/util/flag" | ||
| "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/channels" | ||
| octopusApiClient "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" | ||
| "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/core" | ||
| "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/deployments" | ||
| "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/feeds" | ||
| "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/projects" | ||
|
|
@@ -310,6 +312,14 @@ func createRun(cmd *cobra.Command, f factory.Factory, flags *CreateFlags) error | |
| return err | ||
| } | ||
| options.ProjectName = project.GetName() | ||
|
|
||
| if options.ChannelName != "" { // the executions API only matches channels by name, so resolve any ID we were given | ||
| channel, err := selectors.FindChannel(octopus, project, options.ChannelName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| options.ChannelName = channel.Name | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -318,7 +328,7 @@ func createRun(cmd *cobra.Command, f factory.Factory, flags *CreateFlags) error | |
| executor.NewTask(executor.TaskTypeCreateRelease, options), | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| return DiagnoseCreateReleaseFailure(octopus, options, err) | ||
| } | ||
|
|
||
| if options.Response != nil { | ||
|
|
@@ -420,6 +430,113 @@ func BuildPackageVersionBaselineForChannel(octopus *octopusApiClient.Client, dep | |
| return result, nil | ||
| } | ||
|
|
||
| // DiagnoseCreateReleaseFailure replaces an opaque server-side failure with an actionable message where | ||
| // it can. The server raises a null reference exception, surfaced as a bare 500, when it can't select a | ||
| // version for a package; see https://github.com/OctopusDeploy/cli/issues/426 | ||
| // | ||
| // Any 500 is diagnosed, not just the null reference one, because the message a server sends for this | ||
| // varies by version: current servers report "no viable release plans" instead. The cost of being wrong | ||
| // is bounded, since MissingPackageVersionsError reports what the server actually said alongside the | ||
| // diagnosis. Other 5xx codes are excluded: the failure we are looking for is always raised by the API | ||
| // itself as a 500, so a 502/503/504 is something in front of the server and never worth replaying. | ||
| func DiagnoseCreateReleaseFailure(octopus *octopusApiClient.Client, options *executor.TaskOptionsCreateRelease, cause error) error { | ||
| var apiError *core.APIError | ||
| if !errors.As(cause, &apiError) || apiError.StatusCode != http.StatusInternalServerError { | ||
| return cause | ||
| } | ||
|
|
||
| // 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 { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Consider gating the missing-package diagnosis on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Half of this is already done on this branch, and the other half was tried and deliberately reverted — both on The wrapped cause is now reported. The gate on Live check on this branch, which settles the version-variance argument. Ran So on this server the 500 carries "no viable release plans", not "Object reference not set" — a 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. |
||
| return packages.NewMissingPackageVersionsError(missingPackages, cause) | ||
| } | ||
| } | ||
|
|
||
| if strings.Contains(apiError.ErrorMessage, packages.ServerNullReferenceMessage) { | ||
| return fmt.Errorf("%w\nthe server failed with an unhandled error; this usually means it could not resolve the packages, channel or git reference for the release", cause) | ||
| } | ||
| return cause | ||
| } | ||
|
|
||
| // findPackagesWithoutVersions repeats the package version resolution the server does when it assembles a | ||
| // release, so we can report which packages have no version available in their feed. | ||
| func findPackagesWithoutVersions(octopus *octopusApiClient.Client, options *executor.TaskOptionsCreateRelease) ([]releases.ReleaseTemplatePackage, error) { | ||
| project, err := selectors.FindProject(octopus, options.ProjectName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| gitReferenceKey := "" | ||
| if project.PersistenceSettings != nil && project.PersistenceSettings.Type() == projects.PersistenceSettingsTypeVersionControlled { | ||
| gitReferenceKey = options.GitReference | ||
| if options.GitCommit != "" { // prefer a specific git commit if one was specified | ||
| gitReferenceKey = options.GitCommit | ||
| } | ||
| } | ||
|
|
||
| deploymentProcess, err := octopus.DeploymentProcesses.Get(project, gitReferenceKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| channel, err := findChannelForDiagnosis(octopus, project, options.ChannelName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| deploymentProcessTemplate, err := octopus.DeploymentProcesses.GetTemplate(deploymentProcess, channel.ID, "") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // mirror what the server did: with --ignore-channel-rules it selects versions without applying the | ||
| // channel's version rules, so applying them here would report packages as missing when they only | ||
| // failed the rules. | ||
| var packageVersionBaseline []*packages.StepPackageVersion | ||
| if options.IgnoreChannelRules { | ||
| packageVersionBaseline, err = packages.BuildPackageVersionBaseline(octopus, deploymentProcessTemplate.Packages, nil) | ||
| } else { | ||
| packageVersionBaseline, err = BuildPackageVersionBaselineForChannel(octopus, deploymentProcessTemplate, channel) | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| overrides := packages.BuildPackageVersionOverrides(packageVersionBaseline, options.DefaultPackageVersion, options.PackageVersionOverrides) | ||
| resolvedVersions := packages.ApplyPackageOverrides(packageVersionBaseline, overrides) | ||
|
|
||
| return packages.FindPackagesWithoutVersions(deploymentProcessTemplate.Packages, resolvedVersions), nil | ||
| } | ||
|
|
||
| // findChannelForDiagnosis locates the channel the server would have used. --channel reaches the server as | ||
| // ChannelIDOrName, so we match on either. When no channel was specified we can only guess; the default | ||
| // channel is the best approximation available to us. | ||
| func findChannelForDiagnosis(octopus *octopusApiClient.Client, project *projects.Project, channelIDOrName string) (*channels.Channel, error) { | ||
| existingChannels, err := octopus.Projects.GetChannels(project) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if channelIDOrName != "" { | ||
| for _, c := range existingChannels { | ||
| if strings.EqualFold(c.Name, channelIDOrName) || c.ID == channelIDOrName { | ||
| return c, nil | ||
| } | ||
| } | ||
| return nil, fmt.Errorf("no channel found with name or ID of %s", channelIDOrName) | ||
| } | ||
|
|
||
| if len(existingChannels) == 1 { | ||
| return existingChannels[0], nil | ||
| } | ||
| for _, c := range existingChannels { | ||
| if c.IsDefault { | ||
| return c, nil | ||
| } | ||
| } | ||
| return nil, fmt.Errorf("cannot determine the default channel for project %s", project.GetName()) | ||
| } | ||
|
|
||
| func AskQuestions(octopus *octopusApiClient.Client, stdout io.Writer, asker question.Asker, options *executor.TaskOptionsCreateRelease) error { | ||
| if octopus == nil { | ||
| return cliErrors.NewArgumentNullOrEmptyError("octopus") | ||
|
|
||
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.
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.Responsehandling, whenoctopus.Releases.GetByID(options.Response.ReleaseID)fails, the error branch still dereferences the nil result:ReleaseService.GetByIDreturnsnil, erron 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The finding holds on this branch.
pkg/cmd/release/create/create.go:366-369still readsnewlyCreatedRelease.Assembledand.ReleaseNoteson thelookupErr != nilbranch, andReleaseService.GetByIDreturnsnil, 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 == nilcase, printstime.Time{}, ""instead of reading off the nil, and has arelease creation warns, rather than panicking, when the post-create lookup failsunit 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.