-
Notifications
You must be signed in to change notification settings - Fork 9
fix(attest): don't fail when a CI-defaulted --commit has no repository #1127
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
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -247,6 +247,7 @@ func newAttestJiraCmd(out io.Writer) *cobra.Command { | |||||
| RunE: func(cmd *cobra.Command, args []string) error { | ||||||
| o.repoURLExplicit = cmd.Flags().Changed("repo-url") | ||||||
| o.repoNameExplicit = cmd.Flags().Changed("repository") | ||||||
| o.commitSHAExplicit = cmd.Flags().Changed("commit") | ||||||
| return o.run(args) | ||||||
| }, | ||||||
| } | ||||||
|
|
@@ -292,6 +293,10 @@ func (o *attestJiraOptions) run(args []string) error { | |||||
| return err | ||||||
| } | ||||||
|
|
||||||
| if o.payload.Commit == nil { | ||||||
| return fmt.Errorf("failed to get commit info, which is required to search for Jira issue keys. Pass --commit and point --repo-root at a repository containing it") | ||||||
|
Contributor
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. Good catch on the nil deref (line 304 dereferences The
Suggested change
Also worth noting: whenever this guard passes, lines 300-305 immediately redo the |
||||||
| } | ||||||
|
|
||||||
| gv, err := gitview.New(o.srcRepoRoot) | ||||||
| if err != nil { | ||||||
| return err | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,7 @@ type CommonAttestationOptions struct { | |
| repoProvider string | ||
| repoURLExplicit bool | ||
| repoNameExplicit bool | ||
| commitSHAExplicit bool | ||
| } | ||
|
|
||
| func (o *CommonAttestationOptions) run(args []string, payload *CommonAttestationPayload) error { | ||
|
|
@@ -80,15 +81,10 @@ func (o *CommonAttestationOptions) run(args []string, payload *CommonAttestation | |
| } | ||
|
|
||
| if o.commitSHA != "" { | ||
| gv, err := gitview.New(o.srcRepoRoot) | ||
| payload.Commit, err = resolveCommitInfo(o.srcRepoRoot, o.commitSHA, o.commitSHAExplicit, o.redactedCommitInfo) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get commit info. %s", err) | ||
| return err | ||
| } | ||
| commitInfo, err := gv.GetCommitInfoFromCommitSHA(o.commitSHA, false, o.redactedCommitInfo) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get commit info. %s", err) | ||
| } | ||
| payload.Commit = &commitInfo.BasicCommitInfo | ||
| } | ||
|
|
||
| payload.GitRepoInfo, err = getGitRepoInfoFromEnvironment() | ||
|
|
@@ -117,6 +113,25 @@ func (o *CommonAttestationOptions) run(args []string, payload *CommonAttestation | |
| return err | ||
| } | ||
|
|
||
| // resolveCommitInfo returns nil when git cannot supply the commit info and the | ||
| // commit was not asked for explicitly, so a CI-defaulted --commit does not fail | ||
| // the command in a job with no checked-out repository (#6094). | ||
| func resolveCommitInfo(srcRepoRoot, commitSHA string, explicit bool, redactedCommitInfo []string) (*gitview.BasicCommitInfo, error) { | ||
| gv, err := gitview.New(srcRepoRoot) | ||
| if err == nil { | ||
| var commitInfo *gitview.CommitInfo | ||
| commitInfo, err = gv.GetCommitInfoFromCommitSHA(commitSHA, false, redactedCommitInfo) | ||
| if err == nil { | ||
| return &commitInfo.BasicCommitInfo, nil | ||
| } | ||
| } | ||
| if explicit { | ||
| return nil, fmt.Errorf("failed to get commit info. %s", err) | ||
| } | ||
| logger.Warn("attesting without commit info: --commit defaulted to %s from the CI environment, but %s. Point --repo-root at a repository containing that commit to attach it.", commitSHA, err.Error()) | ||
|
Comment on lines
+116
to
+131
Contributor
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. Two things about the soft-fail branch: 1. The warning is the only signal a user gets that commit info is now missing. In CI it competes with everything else on stderr, and the consequence isn't stated: the 2. A wrong Minor: |
||
| return nil, nil | ||
| } | ||
|
|
||
| // mergeGitRepoInfo applies flag overrides onto base (which may be nil) and | ||
| // returns nil if ID, Name, or URL is still empty after merging, so that the | ||
| // field is omitted from the JSON payload. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "testing" | ||
|
|
||
| "github.com/go-git/go-git/v5" | ||
| "github.com/stretchr/testify/suite" | ||
| ) | ||
|
|
||
| // CommitInfoResolutionTestSuite guards that a --commit which was defaulted from | ||
| // the CI environment does not fail the command when git cannot supply its info, | ||
| // while an explicitly passed --commit still does. | ||
| // | ||
| // The production trigger (a CI-defaulted --commit in a job with no checked-out | ||
| // repository) cannot be reproduced through the command harness, because | ||
| // DefaultValue returns "" whenever KOSLI_TESTS is set. resolveCommitInfo is | ||
| // therefore exercised directly, and the command cases below guard only that | ||
| // each command assigns commitSHAExplicit. | ||
|
Comment on lines
+14
to
+19
Contributor
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 stated limitation isn't quite true — the harness can produce a CI-defaulted That makes an end-to-end case possible, and it is the one behaviour nothing currently covers — that the command exits 0 and emits the warning: value, inTests := os.LookupEnv("KOSLI_TESTS")
require.NoError(suite.T(), os.Unsetenv("KOSLI_TESTS"))
defer func() { if inTests { os.Setenv("KOSLI_TESTS", value) } }()
suite.T().Setenv("GITHUB_RUN_NUMBER", "1") // WhichCI() -> github
suite.T().Setenv("GITHUB_SHA", suite.headHash)
// attest generic ... --repo-root testdata, no --commit
// expect: no error, stderr contains "attesting without commit info"Without it, the warning text — the only thing the user sees on the new path — is unasserted, and nothing catches a regression that turns the soft-fail back into an error at the command level. Nit: |
||
| type CommitInfoResolutionTestSuite struct { | ||
| suite.Suite | ||
| headHash string | ||
| defaultKosliArguments string | ||
| } | ||
|
|
||
| func (suite *CommitInfoResolutionTestSuite) SetupTest() { | ||
| repo, err := git.PlainOpen("../..") | ||
| suite.Require().NoError(err) | ||
| head, err := repo.Head() | ||
| suite.Require().NoError(err) | ||
| suite.headHash = head.Hash().String() | ||
|
|
||
| global = &GlobalOpts{ | ||
| ApiToken: "DRY_RUN", | ||
| Org: "test-org", | ||
| Host: "http://localhost:8001", | ||
| DryRun: true, | ||
| } | ||
| suite.defaultKosliArguments = " --dry-run --host http://localhost:8001 --org test-org --api-token DRY_RUN" | ||
| } | ||
|
|
||
| func (suite *CommitInfoResolutionTestSuite) TestResolveCommitInfoWithoutRepository() { | ||
| const noRepo = "testdata" | ||
|
|
||
| info, err := resolveCommitInfo(noRepo, suite.headHash, false, []string{}) | ||
| suite.Require().NoError(err, "a CI-defaulted commit must not fail when there is no repository") | ||
| suite.Nil(info) | ||
|
|
||
| _, err = resolveCommitInfo(noRepo, suite.headHash, true, []string{}) | ||
| suite.Require().Error(err, "an explicit --commit must still fail when there is no repository") | ||
| suite.Contains(err.Error(), "repository does not exist") | ||
| } | ||
|
|
||
| func (suite *CommitInfoResolutionTestSuite) TestResolveCommitInfoWithUnresolvableCommit() { | ||
| // A well-formed SHA that is not in this repository, as in a shallow clone. | ||
| const absentSHA = "0d4c1e1b7f5c2a9e8b3d6f0a1c4e7b2d5a8f3c60" | ||
|
|
||
| info, err := resolveCommitInfo("../..", absentSHA, false, []string{}) | ||
| suite.Require().NoError(err, "a CI-defaulted commit must not fail when it cannot be resolved") | ||
| suite.Nil(info) | ||
|
|
||
| _, err = resolveCommitInfo("../..", absentSHA, true, []string{}) | ||
| suite.Require().Error(err, "an explicit --commit must still fail when it cannot be resolved") | ||
| } | ||
|
|
||
| func (suite *CommitInfoResolutionTestSuite) TestResolveCommitInfoSucceeds() { | ||
| info, err := resolveCommitInfo("../..", suite.headHash, false, []string{}) | ||
| suite.Require().NoError(err) | ||
| suite.Require().NotNil(info) | ||
| suite.Equal(suite.headHash, info.Sha1) | ||
| } | ||
|
|
||
| func (suite *CommitInfoResolutionTestSuite) TestExplicitCommitWiring() { | ||
| tests := []cmdTestCase{ | ||
| { | ||
| wantError: true, | ||
| name: "attest generic: an explicit --commit fails when --repo-root has no repository", | ||
| cmd: fmt.Sprintf("attest generic --fingerprint 7509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9 --name foo --flow f --trail t --commit %s --repo-root testdata%s", suite.headHash, suite.defaultKosliArguments), | ||
| goldenRegex: "Error: failed to get commit info\\. .*repository does not exist\n", | ||
| }, | ||
| { | ||
| wantError: true, | ||
| name: "begin trail: an explicit --commit fails when --repo-root has no repository", | ||
| cmd: fmt.Sprintf("begin trail t --flow f --commit %s --repo-root testdata%s", suite.headHash, suite.defaultKosliArguments), | ||
| goldenRegex: "Error: failed to get commit info\\. .*repository does not exist\n", | ||
| }, | ||
| } | ||
| runTestCmd(suite.T(), tests) | ||
| } | ||
|
|
||
| // commitRequiredOptions builds the shared attestation options for a command run | ||
| // whose --commit came from the CI default and cannot be resolved, which is the | ||
| // only way payload.Commit reaches these commands as nil. | ||
| func (suite *CommitInfoResolutionTestSuite) commitRequiredOptions() *CommonAttestationOptions { | ||
| return &CommonAttestationOptions{ | ||
| fingerprintOptions: &fingerprintOptions{}, | ||
| attestationNameTemplate: "foo", | ||
| flowName: "f", | ||
| trailName: "t", | ||
| commitSHA: suite.headHash, | ||
| srcRepoRoot: "testdata", | ||
| commitSHAExplicit: false, | ||
| } | ||
| } | ||
|
|
||
| func (suite *CommitInfoResolutionTestSuite) TestCommandsNeedingCommitReportIt() { | ||
| pr := &attestPROptions{ | ||
| CommonAttestationOptions: suite.commitRequiredOptions(), | ||
| payload: PRAttestationPayload{CommonAttestationPayload: &CommonAttestationPayload{}}, | ||
| } | ||
| err := pr.run([]string{}) | ||
| suite.Require().Error(err) | ||
| suite.Contains(err.Error(), "required to find pull requests") | ||
|
|
||
| jira := &attestJiraOptions{ | ||
| CommonAttestationOptions: suite.commitRequiredOptions(), | ||
| payload: JiraAttestationPayload{CommonAttestationPayload: &CommonAttestationPayload{}}, | ||
| } | ||
| err = jira.run([]string{}) | ||
| suite.Require().Error(err) | ||
| suite.Contains(err.Error(), "required to search for Jira issue keys") | ||
| } | ||
|
|
||
| func TestCommitInfoResolutionTestSuite(t *testing.T) { | ||
| suite.Run(t, new(CommitInfoResolutionTestSuite)) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,10 +53,15 @@ func GithubPRNumber() int { | |
| } | ||
|
|
||
| func CloneGitRepo(url, cloneTo string) (*git.Repository, error) { | ||
| // Resolved for the same reason as in InitializeGitRepo below. | ||
| resolvedCloneTo, err := filepath.EvalSymlinks(cloneTo) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // the repo worktree filesystem. It has to be osfs so that we can give it a path | ||
|
Comment on lines
+56
to
61
Contributor
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 diagnosis is right and matches |
||
| fs := osfs.New(cloneTo) | ||
| fs := osfs.New(resolvedCloneTo) | ||
| // the filesystem for git database | ||
| storerFS := osfs.New(filepath.Join(cloneTo, ".git")) | ||
| storerFS := osfs.New(filepath.Join(resolvedCloneTo, ".git")) | ||
| storer := filesystem.NewStorage(storerFS, cache.NewObjectLRUDefault()) | ||
| return git.Clone(storer, fs, &git.CloneOptions{URL: url}) | ||
| } | ||
|
|
||
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.
This line is now repeated in 13
RunEbodies. It follows the existingrepoURLExplicit/repoNameExplicitpattern, so it's consistent — but the failure mode is new and silent: a futureattestcommand that embedsCommonAttestationOptionsand forgets this line still compiles, still passes tests, and quietly downgrades an explicit--commitfrom a hard error to a warning. Nothing catches it.addAttestationFlags(cmd, o.CommonAttestationOptions, ...)already receives bothcmdando, so it could own the assignment for all three flags — e.g. by chaining aPreRunEthere, or by stashingo.flags = cmd.Flags()and readingChangedat use-site. Worth doing here or in a follow-up, and worth a note in.claude/skills/new-command/either way, since that skill scaffolds new attest commands.