Skip to content
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.

Sign up for GitHub

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

Fix SBOM nightly failures #3449

Merged
merged 3 commits into from
Aug 16, 2024
Merged

Fix SBOM nightly failures #3449

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
528fb3e
Add separate stagecommit method
MDrakos Aug 15, 2024
c1dc2a2
Return partial commit on failure
MDrakos Aug 16, 2024
80a3671
Fix panic
MDrakos Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions internal/runners/packages/import.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,28 +125,32 @@ func (i *Import) Run(params *ImportRunParams) (rerr error) {
return locale.WrapError(err, "err_cannot_apply_changeset", "Could not apply changeset")
}

solveSpinner := output.StartSpinner(i.prime.Output(), locale.T("progress_solve_preruntime"), constants.TerminalAnimationInterval)
msg := locale.T("commit_reqstext_message")
stagedCommit, err := bp.StageCommit(buildplanner.StageCommitParams{
stagedCommitID, err := bp.StageCommitWithoutBuild(buildplanner.StageCommitParams{
Owner: proj.Owner(),
Project: proj.Name(),
ParentCommit: localCommitId.String(),
Description: msg,
Script: bs,
})
if err != nil {
solveSpinner.Stop(locale.T("progress_fail"))
return locale.WrapError(err, "err_commit_changeset", "Could not commit import changes")
}

pg.Stop(locale.T("progress_success"))
pg = nil

if err := localcommit.Set(proj.Dir(), stagedCommit.CommitID.String()); err != nil {
solveSpinner.Stop(locale.T("progress_fail"))
if err := localcommit.Set(proj.Dir(), stagedCommitID.String()); err != nil {
return locale.WrapError(err, "err_package_update_commit_id")
}

solveSpinner := output.StartSpinner(i.prime.Output(), locale.T("progress_solve_preruntime"), constants.TerminalAnimationInterval)
stagedCommit, err := bp.FetchCommit(stagedCommitID, proj.Owner(), proj.Name(), nil)
if err != nil {
solveSpinner.Stop(locale.T("progress_fail"))
return errs.Wrap(err, "Failed to fetch build result for previous commit")
}

// Output change summary.
previousCommit, err := bp.FetchCommit(localCommitId, proj.Owner(), proj.Name(), nil)
if err != nil {
Expand Down
39 changes: 39 additions & 0 deletions pkg/platform/model/buildplanner/commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,45 @@ func (b *BuildPlanner) StageCommit(params StageCommitParams) (*Commit, error) {
return &Commit{resp.Commit, bp, stagedScript}, nil
}

// StageCommitWithoutBuild stages a commit but does not request the corresponding build plan or
// build expression. This is useful when we want to stage a commit but we know that the commit may
// not solve successfully. This funciton should be used sparingly as requesting a build after calling
// this function will result in excess solves.
func (b *BuildPlanner) StageCommitWithoutBuild(params StageCommitParams) (strfmt.UUID, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a little weird. It seems the issue is that the error we run into otherwise comes from some place that happens after we go beyond the "planned" stage of a build. So in effect what we are doing here is not avoiding buildscripts or buildplans so much as we're avoiding any state beyond "planned".

My problem with this is that it's very implicit behaviour. We basically expect the API to ALWAYS respond with "planned" at this stage, which may very well be true but that feels like a shaky contract to rely on.

I think what we need here is to do a normal stage commit, but to ensure that we receive a commit back regardless of whether it succeeded, so we can choose what to do with the result. Is that currently not happening with the StageCommit() function?

Basically I'm saying we should always return the commit here:

cli/pkg/platform/model/buildplanner/commit.go

Line 76 in c8c14b6

return nil, errs.Wrap(err, "failed to poll build plan")

Then consuming code (in this case the import runner) sees an error occurs, and it will error out but not before saving the commit, provided it is non-empty (meaning the error did not stop the commit from happening).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the status of the build isn't planned, it outright failed. The reason I added this function is because the caller knowingly wants to disregard the build and just take the staged commit regardless.

Here is an example of the response we are getting from the API in this case:

{
    "data": {
        "stageCommit": {
            "__typename": "Commit",
            "expr": {
                ...
            },
            "commitId": "02a1fa15-90fd-44dc-9a5f-25918b8753ea",
            "build": {
                "__typename": "PlanningError",
                "message": "Failed to create build plan",
                "subErrors": [
                    {
                        "__typename": "GenericSolveError",
                        "path": ".solve_legacy",
                        "message": "The requested requirements refer to 1 non-existent namespace: private/ActiveState-CLI",
                        "isTransient": true,
                        "validationErrors": []
                    }
                ]
            }
        }
    }
}

I would prefer to avoid using a return value when there is an error but that is just a preference. If you want to go that route we would have to return a partial Commit object here:

cli/pkg/platform/model/buildplanner/commit.go

Line 67 in c8c14b6

return nil, response.ProcessBuildError(resp.Commit.Build, "Could not process error response from stage commit")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the API in this case still return the commit, or does it just return the error?

if it’s not planned and outright failed then I don’t understand how the new stagecommit function differs? Sounds like either variant would run into the same error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the commit is returned which contains the commit ID and the build. The failure is in the build, which this function doesn't inspect because it only wants to return the staged commit ID.

We are reverting to the old way of using StageCommit for import as we want to update the project's local commit ID regardless of any build failures. In the case of CycloneDX and SPDX we are expecting this command to fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By introducing the new function we're duplicating logic that will need to be maintained. And it's all to avoid an error being returned, which at that point isn't much different from ignoring the error, except that my suggestion has us explicitly ignoring it and the current solution implicitly avoids it.

I would prefer we go with the approach I described; do not introduce a new StageCommit function, instead always update the commit even if StageCommit() returns an error (provided it did return a commit, of course).

Note that returning a result alongside an error and interpreting it is still idiomatic. It's about giving the full information to the consuming code to let it decide how to handle this scenario. I tried to find links explaining this pattern, but can't find anything explicitly touching on this subject. That said you can find plenty of examples of it in Go's standard library, eg. os.Read().

logging.Debug("StageCommit, params: %+v", params)
script := params.Script

if script == nil {
return "", errs.New("Script is nil")
}

expression, err := script.MarshalBuildExpression()
if err != nil {
return "", errs.Wrap(err, "Failed to marshal build expression")
}

// With the updated build expression call the stage commit mutation
request := request.StageCommit(params.Owner, params.Project, params.ParentCommit, params.Description, script.AtTime(), expression)
resp := &response.StageCommitResult{}
if err := b.client.Run(request, resp); err != nil {
return "", processBuildPlannerError(err, "failed to stage commit")
}

if resp.Commit == nil {
return "", errs.New("Staged commit is nil")
}

if response.IsErrorResponse(resp.Commit.Type) {
return "", response.ProcessCommitError(resp.Commit, "Could not process error response from stage commit")
}

if resp.Commit.CommitID == "" {
return "", errs.New("Staged commit does not contain commitID")
}

return resp.Commit.CommitID, nil
}

func (b *BuildPlanner) RevertCommit(organization, project, parentCommitID, commitID string) (strfmt.UUID, error) {
logging.Debug("RevertCommit, organization: %s, project: %s, commitID: %s", organization, project, commitID)
resp := &response.RevertCommitResult{}
Expand Down
Loading