Fix SBOM nightly failures

[MDrakos]

DX-3008 Nightly failure: CycloneDX and SPDX SBOM imports do not create commits

[MDrakos]

Attempting to introduce an error case to the existing StageCommit function and use a partial Commit object in the runner started to feel awkward so I opted for another stage commit function that doesn't request the build. I believe it makes sense here as we have a state import use-case that we know will fail.

[Naatan]

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).

[MDrakos]

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")

[Naatan]

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.

[MDrakos]

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.

[Naatan]

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().

[Naatan]

#3449 (comment)

[Naatan]

Looks good! I'm not sure how we're gonna resolve the commit/solve spinner conflict, but that's another story 😅

[Naatan]

Hit wrong button