From ce58295ac3ee43d448902fb0e6ccd35ef70aeecb Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Fri, 1 Dec 2023 11:34:31 +0100 Subject: [PATCH 1/4] fix(cli attestation): handle non-merged branches Signed-off-by: Miguel Martinez Trivino --- internal/attestation/crafter/crafter.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index 117180e91..5ed52852c 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -293,7 +293,9 @@ func gracefulGitRepoHead(path string) (*HeadCommit, error) { remotes, err := repo.Remotes() if err != nil { - return nil, fmt.Errorf("getting remotes: %w", err) + // go-git does an additional validation that the branch is pushed upstream + // we do not care about that use-case, so we ignore the error + return c, nil } for _, r := range remotes { From 91d7ee024cc898d099bb7cf57ac9e4d0c4993f37 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Fri, 1 Dec 2023 11:41:34 +0100 Subject: [PATCH 2/4] fix(cli attestation): handle non-merged branches Signed-off-by: Miguel Martinez Trivino --- internal/attestation/crafter/crafter.go | 17 +++++++++-------- .../attestation/crafter/crafter_unit_test.go | 2 +- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index 5ed52852c..49ca54f74 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -186,7 +186,7 @@ func LoadSchema(pathOrURI string) (*schemaapi.CraftingSchema, error) { // Initialize the temporary file with the content of the schema func (c *Crafter) initCraftingStateFile(schema *schemaapi.CraftingSchema, wf *api.WorkflowMetadata, dryRun bool, runnerType schemaapi.CraftingSchema_Runner_RunnerType, jobURL string) error { // Generate Crafting state - state, err := initialCraftingState(c.workingDir, schema, wf, dryRun, runnerType, jobURL) + state, err := c.initialCraftingState(c.workingDir, schema, wf, dryRun, runnerType, jobURL) if err != nil { return fmt.Errorf("initializing crafting state: %w", err) } @@ -255,7 +255,7 @@ type CommitRemote struct { // Returns the current directory git commit hash if possible // If we are not in a git repo it will return an empty string -func gracefulGitRepoHead(path string) (*HeadCommit, error) { +func (c *Crafter) gracefulGitRepoHead(path string) (*HeadCommit, error) { repo, err := git.PlainOpenWithOptions(path, &git.PlainOpenOptions{ // walk up the directory tree until we find a git repo DetectDotGit: true, @@ -282,7 +282,7 @@ func gracefulGitRepoHead(path string) (*HeadCommit, error) { return nil, fmt.Errorf("finding head commit: %w", err) } - c := &HeadCommit{ + headCommit := &HeadCommit{ Hash: commit.Hash.String(), AuthorEmail: commit.Author.Email, AuthorName: commit.Author.Name, @@ -293,9 +293,10 @@ func gracefulGitRepoHead(path string) (*HeadCommit, error) { remotes, err := repo.Remotes() if err != nil { + c.logger.Warn().Err(err).Msg("failed to list remotes") // go-git does an additional validation that the branch is pushed upstream // we do not care about that use-case, so we ignore the error - return c, nil + return headCommit, nil } for _, r := range remotes { @@ -303,18 +304,18 @@ func gracefulGitRepoHead(path string) (*HeadCommit, error) { continue } - c.Remotes = append(c.Remotes, &CommitRemote{ + headCommit.Remotes = append(headCommit.Remotes, &CommitRemote{ Name: r.Config().Name, URL: r.Config().URLs[0], }) } - return c, nil + return headCommit, nil } -func initialCraftingState(cwd string, schema *schemaapi.CraftingSchema, wf *api.WorkflowMetadata, dryRun bool, runnerType schemaapi.CraftingSchema_Runner_RunnerType, jobURL string) (*api.CraftingState, error) { +func (c *Crafter) initialCraftingState(cwd string, schema *schemaapi.CraftingSchema, wf *api.WorkflowMetadata, dryRun bool, runnerType schemaapi.CraftingSchema_Runner_RunnerType, jobURL string) (*api.CraftingState, error) { // Get git commit hash - headCommit, err := gracefulGitRepoHead(cwd) + headCommit, err := c.gracefulGitRepoHead(cwd) if err != nil { return nil, fmt.Errorf("getting git commit hash: %w", err) } diff --git a/internal/attestation/crafter/crafter_unit_test.go b/internal/attestation/crafter/crafter_unit_test.go index 7ceb01d31..aec0b39da 100644 --- a/internal/attestation/crafter/crafter_unit_test.go +++ b/internal/attestation/crafter/crafter_unit_test.go @@ -121,7 +121,7 @@ func (s *crafterUnitSuite) TestGitRepoHead() { require.NoError(s.T(), err) } - got, err := gracefulGitRepoHead(path) + got, err := NewCrafter().gracefulGitRepoHead(path) if tc.wantErr { assert.Error(s.T(), err) return From 22fed7f26d25ad5edd7f1903779bd4fe71d13329 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Fri, 1 Dec 2023 12:02:24 +0100 Subject: [PATCH 3/4] fix(cli attestation): handle non-merged branches Signed-off-by: Miguel Martinez Trivino --- internal/attestation/crafter/crafter.go | 22 +++++++++++++------ .../attestation/crafter/crafter_unit_test.go | 2 +- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index 49ca54f74..75512586e 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -253,9 +253,12 @@ type CommitRemote struct { Name, URL string } +// This error is not exposed by go-git +var errBranchInvalidMerge = errors.New("branch config: invalid merge") + // Returns the current directory git commit hash if possible // If we are not in a git repo it will return an empty string -func (c *Crafter) gracefulGitRepoHead(path string) (*HeadCommit, error) { +func gracefulGitRepoHead(path string) (*HeadCommit, error) { repo, err := git.PlainOpenWithOptions(path, &git.PlainOpenOptions{ // walk up the directory tree until we find a git repo DetectDotGit: true, @@ -282,7 +285,7 @@ func (c *Crafter) gracefulGitRepoHead(path string) (*HeadCommit, error) { return nil, fmt.Errorf("finding head commit: %w", err) } - headCommit := &HeadCommit{ + c := &HeadCommit{ Hash: commit.Hash.String(), AuthorEmail: commit.Author.Email, AuthorName: commit.Author.Name, @@ -293,10 +296,15 @@ func (c *Crafter) gracefulGitRepoHead(path string) (*HeadCommit, error) { remotes, err := repo.Remotes() if err != nil { - c.logger.Warn().Err(err).Msg("failed to list remotes") // go-git does an additional validation that the branch is pushed upstream // we do not care about that use-case, so we ignore the error - return headCommit, nil + // we compare by error string because go-git does not expose the error type + // and errors.Is require the same instance of the error + if err.Error() == errBranchInvalidMerge.Error() { + return c, nil + } + + return nil, fmt.Errorf("getting remotes: %w", err) } for _, r := range remotes { @@ -304,18 +312,18 @@ func (c *Crafter) gracefulGitRepoHead(path string) (*HeadCommit, error) { continue } - headCommit.Remotes = append(headCommit.Remotes, &CommitRemote{ + c.Remotes = append(c.Remotes, &CommitRemote{ Name: r.Config().Name, URL: r.Config().URLs[0], }) } - return headCommit, nil + return c, nil } func (c *Crafter) initialCraftingState(cwd string, schema *schemaapi.CraftingSchema, wf *api.WorkflowMetadata, dryRun bool, runnerType schemaapi.CraftingSchema_Runner_RunnerType, jobURL string) (*api.CraftingState, error) { // Get git commit hash - headCommit, err := c.gracefulGitRepoHead(cwd) + headCommit, err := gracefulGitRepoHead(cwd) if err != nil { return nil, fmt.Errorf("getting git commit hash: %w", err) } diff --git a/internal/attestation/crafter/crafter_unit_test.go b/internal/attestation/crafter/crafter_unit_test.go index aec0b39da..7ceb01d31 100644 --- a/internal/attestation/crafter/crafter_unit_test.go +++ b/internal/attestation/crafter/crafter_unit_test.go @@ -121,7 +121,7 @@ func (s *crafterUnitSuite) TestGitRepoHead() { require.NoError(s.T(), err) } - got, err := NewCrafter().gracefulGitRepoHead(path) + got, err := gracefulGitRepoHead(path) if tc.wantErr { assert.Error(s.T(), err) return From 40b140adae736627a52b33c631b8ed5ca0e04035 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Fri, 1 Dec 2023 12:03:05 +0100 Subject: [PATCH 4/4] fix(cli attestation): handle non-merged branches Signed-off-by: Miguel Martinez Trivino --- internal/attestation/crafter/crafter.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index 75512586e..cb0ef98b1 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -186,7 +186,7 @@ func LoadSchema(pathOrURI string) (*schemaapi.CraftingSchema, error) { // Initialize the temporary file with the content of the schema func (c *Crafter) initCraftingStateFile(schema *schemaapi.CraftingSchema, wf *api.WorkflowMetadata, dryRun bool, runnerType schemaapi.CraftingSchema_Runner_RunnerType, jobURL string) error { // Generate Crafting state - state, err := c.initialCraftingState(c.workingDir, schema, wf, dryRun, runnerType, jobURL) + state, err := initialCraftingState(c.workingDir, schema, wf, dryRun, runnerType, jobURL) if err != nil { return fmt.Errorf("initializing crafting state: %w", err) } @@ -321,7 +321,7 @@ func gracefulGitRepoHead(path string) (*HeadCommit, error) { return c, nil } -func (c *Crafter) initialCraftingState(cwd string, schema *schemaapi.CraftingSchema, wf *api.WorkflowMetadata, dryRun bool, runnerType schemaapi.CraftingSchema_Runner_RunnerType, jobURL string) (*api.CraftingState, error) { +func initialCraftingState(cwd string, schema *schemaapi.CraftingSchema, wf *api.WorkflowMetadata, dryRun bool, runnerType schemaapi.CraftingSchema_Runner_RunnerType, jobURL string) (*api.CraftingState, error) { // Get git commit hash headCommit, err := gracefulGitRepoHead(cwd) if err != nil {