diff --git a/.chloggen/codeboten_review-feedback.yaml b/.chloggen/codeboten_review-feedback.yaml new file mode 100755 index 00000000..8201ca37 --- /dev/null +++ b/.chloggen/codeboten_review-feedback.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: breaking + +# The name of the component, or a single word describing the area of concern, (e.g. crosslink) +component: multimod + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Update HasChanged to avoid returning unnecessary boolean + +# One or more tracking issues related to the change +issues: [366] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: diff --git a/multimod/cmd/diff.go b/multimod/cmd/diff.go index ae23d369..63c54048 100644 --- a/multimod/cmd/diff.go +++ b/multimod/cmd/diff.go @@ -26,12 +26,12 @@ var diffCmd = &cobra.Command{ log.Fatalf("could not find repo root: %v", err) } - changed, changedFiles, err := diff.HasChanged(repoRoot, versioningFile, previousVersion, moduleSetName) + changedFiles, err := diff.HasChanged(repoRoot, versioningFile, previousVersion, moduleSetName) if err != nil { log.Fatalf("error running diff: %v", err) } - if changed { + if len(changedFiles) > 0 { log.Fatalf("The following files changed in %s modules since %s: \n%s\nRelease is required for %s modset", moduleSetName, previousVersion, strings.Join(changedFiles, "\n"), moduleSetName) } log.Printf("No %s modules have changed since %s", moduleSetName, previousVersion) diff --git a/multimod/internal/common/git.go b/multimod/internal/common/git.go index a6526121..c2283827 100644 --- a/multimod/internal/common/git.go +++ b/multimod/internal/common/git.go @@ -18,7 +18,6 @@ import ( "errors" "fmt" "log" - "strings" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" @@ -156,53 +155,3 @@ func VerifyWorkingTreeClean(repo *git.Repository) error { return nil } - -type Client interface { - HeadCommit(r *git.Repository) (*object.Commit, error) - TagCommit(r *git.Repository, tag string) (*object.Commit, error) - FilesChanged(headCommit *object.Commit, tagCommit *object.Commit, prefix string, suffix string) ([]string, error) -} - -type GitClient struct{} - -func (g GitClient) HeadCommit(r *git.Repository) (*object.Commit, error) { - headRef, err := r.Head() - if err != nil { - return nil, err - } - return r.CommitObject(headRef.Hash()) -} - -func (g GitClient) TagCommit(r *git.Repository, tag string) (*object.Commit, error) { - tagRef, err := r.Tag(tag) - if err != nil { - return nil, err - } - - o, err := r.TagObject(tagRef.Hash()) - if err != nil { - return nil, fmt.Errorf("tag object error %s %w", tagRef.Hash().String(), err) - } - return r.CommitObject(o.Target) -} - -// FilesChanged returns the differences between two commits -func (g GitClient) FilesChanged(headCommit *object.Commit, tagCommit *object.Commit, prefix string, suffix string) ([]string, error) { - changedFiles := []string{} - p, err := headCommit.Patch(tagCommit) - if err != nil { - return changedFiles, err - } - - for _, f := range p.FilePatches() { - from, to := f.Files() - if from != nil && strings.HasSuffix(from.Path(), suffix) && strings.HasPrefix(from.Path(), prefix) { - changedFiles = append(changedFiles, from.Path()) - continue - } - if to != nil && strings.HasSuffix(to.Path(), suffix) && strings.HasPrefix(to.Path(), prefix) { - changedFiles = append(changedFiles, to.Path()) - } - } - return changedFiles, nil -} diff --git a/multimod/internal/diff/diff.go b/multimod/internal/diff/diff.go index 4bc63636..d2a5b840 100644 --- a/multimod/internal/diff/diff.go +++ b/multimod/internal/diff/diff.go @@ -10,13 +10,64 @@ import ( "strings" "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing/object" "go.opentelemetry.io/build-tools/multimod/internal/common" ) -// check +type Client interface { + HeadCommit(r *git.Repository) (*object.Commit, error) + TagCommit(r *git.Repository, tag string) (*object.Commit, error) + FilesChanged(headCommit *object.Commit, tagCommit *object.Commit, prefix string, suffix string) ([]string, error) +} + +type GitClient struct{} + +func (g GitClient) HeadCommit(r *git.Repository) (*object.Commit, error) { + headRef, err := r.Head() + if err != nil { + return nil, err + } + return r.CommitObject(headRef.Hash()) +} -// normalizeVersion ensures the version is prefixed with a `v`. +func (g GitClient) TagCommit(r *git.Repository, tag string) (*object.Commit, error) { + tagRef, err := r.Tag(tag) + if err != nil { + return nil, err + } + + o, err := r.TagObject(tagRef.Hash()) + if err != nil { + return nil, fmt.Errorf("tag object error %s %w", tagRef.Hash().String(), err) + } + return r.CommitObject(o.Target) +} + +// FilesChanged returns a list of files that have changed between two commits. +func (g GitClient) FilesChanged(headCommit *object.Commit, tagCommit *object.Commit, prefix string, suffix string) ([]string, error) { + changedFiles := []string{} + p, err := headCommit.Patch(tagCommit) + if err != nil { + return changedFiles, err + } + + for _, f := range p.FilePatches() { + from, to := f.Files() + if from != nil && strings.HasSuffix(from.Path(), suffix) && strings.HasPrefix(from.Path(), prefix) { + changedFiles = append(changedFiles, from.Path()) + continue + } + if to != nil && strings.HasSuffix(to.Path(), suffix) && strings.HasPrefix(to.Path(), prefix) { + changedFiles = append(changedFiles, to.Path()) + } + } + return changedFiles, nil +} + +// normalizeVersion ensures the version is prefixed with a `v`. The missing v prefix in +// the version has caused problems in the collector repo. This logic was originall implemented +// in the Makefile. func normalizeVersion(ver string) string { if strings.HasPrefix(ver, "v") { return ver @@ -31,23 +82,22 @@ func normalizeTag(tagName common.ModuleTagName, ver string) string { return fmt.Sprintf("%s/%s", tagName, ver) } -func HasChanged(repoRoot string, versioningFile string, ver string, modset string) (bool, []string, error) { - changed := false +func HasChanged(repoRoot string, versioningFile string, ver string, modset string) ([]string, error) { changedFiles := []string{} ver = normalizeVersion(ver) r, err := git.PlainOpen(repoRoot) if err != nil { - return changed, changedFiles, fmt.Errorf("could not open repo at %v: %w", repoRoot, err) + return changedFiles, fmt.Errorf("could not open repo at %v: %w", repoRoot, err) } if e := common.VerifyWorkingTreeClean(r); e != nil { - return changed, changedFiles, fmt.Errorf("VerifyWorkingTreeClean failed: %w", e) + return changedFiles, fmt.Errorf("VerifyWorkingTreeClean failed: %w", e) } mset, err := common.NewModuleSetRelease(versioningFile, modset, repoRoot) if err != nil { - return changed, changedFiles, err + return changedFiles, err } // get tag names of mods to update @@ -57,44 +107,40 @@ func HasChanged(repoRoot string, versioningFile string, ver string, modset strin repoRoot, ) if err != nil { - return changed, changedFiles, fmt.Errorf("could not retrieve tag names from module paths: %w", err) + return changedFiles, fmt.Errorf("could not retrieve tag names from module paths: %w", err) } - return filesChanged(r, modset, ver, tagNames, common.GitClient{}) + return filesChanged(r, modset, ver, tagNames, GitClient{}) } -func filesChanged(r *git.Repository, modset string, ver string, tagNames []common.ModuleTagName, client common.Client) (bool, []string, error) { - changed := false +func filesChanged(r *git.Repository, modset string, ver string, tagNames []common.ModuleTagName, client Client) ([]string, error) { changedFiles := []string{} headCommit, err := client.HeadCommit(r) if err != nil { - return changed, changedFiles, err + return changedFiles, err } // get all modules in modset for _, tagName := range tagNames { - // check tag exists tag := normalizeTag(tagName, ver) - tagCommit, err := client.TagCommit(r, tag) if err != nil { if errors.Is(err, git.ErrTagNotFound) { log.Printf("Module %s does not have a %s tag", tagName, ver) log.Printf("%s release is required.", modset) - return changed, changedFiles, fmt.Errorf("tag not found %s", tag) + return changedFiles, fmt.Errorf("tag not found %s", tag) } - return changed, changedFiles, err + return changedFiles, err } files, err := client.FilesChanged(headCommit, tagCommit, string(tagName), ".go") if err != nil { - return changed, changedFiles, err + return changedFiles, err } if len(files) > 0 { - changed = true changedFiles = append(changedFiles, files...) } } - return changed, changedFiles, nil + return changedFiles, nil } diff --git a/multimod/internal/diff/diff_test.go b/multimod/internal/diff/diff_test.go index 7decba39..e7a0947f 100644 --- a/multimod/internal/diff/diff_test.go +++ b/multimod/internal/diff/diff_test.go @@ -67,34 +67,29 @@ func TestHasChanged(t *testing.T) { modset string versionsFile string repoRoot string - expected bool err error }{ { name: "invalid repoRoot", - expected: false, err: errors.New("repository does not exist"), tag: "v0.8.0", modset: "tools", repoRoot: "invalid-repo-root", }, { - name: "invalid tag", - expected: false, - err: errors.New("tag not found"), - tag: "1.2.3", - modset: "tools", + name: "invalid tag", + err: errors.New("tag not found"), + tag: "1.2.3", + modset: "tools", }, { - name: "invalid modset", - expected: false, - err: errors.New("could not find module set"), - tag: "v0.8.0", - modset: "invalid", + name: "invalid modset", + err: errors.New("could not find module set"), + tag: "v0.8.0", + modset: "invalid", }, { name: "invalid versions file", - expected: false, err: errors.New("no such file or directory"), tag: "v0.8.0", versionsFile: "invalid.yaml", @@ -119,19 +114,14 @@ func TestHasChanged(t *testing.T) { } else { versionFile = filepath.Join(repoRoot, "versions.yaml") } - actual, changedFiles, err := HasChanged(repoRoot, versionFile, tt.tag, tt.modset) + changedFiles, err := HasChanged(repoRoot, versionFile, tt.tag, tt.modset) if tt.err != nil { require.Error(t, err) require.ErrorContains(t, err, tt.err.Error()) } else { require.NoError(t, err) } - require.Equal(t, tt.expected, actual) - if actual { - require.True(t, len(changedFiles) > 0) - } else { - require.False(t, len(changedFiles) > 0) - } + require.False(t, len(changedFiles) > 0) }) } } @@ -194,19 +184,14 @@ func TestFilesChanged(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - actual, changedFiles, err := filesChanged(nil, tt.modset, tt.version, tt.tagNames, tt.cli) + changedFiles, err := filesChanged(nil, tt.modset, tt.version, tt.tagNames, tt.cli) if tt.err != nil { require.Error(t, err) require.ErrorContains(t, err, tt.err.Error()) } else { require.NoError(t, err) } - require.Equal(t, tt.expected, actual) - if actual { - require.True(t, len(changedFiles) > 0) - } else { - require.False(t, len(changedFiles) > 0) - } + require.Equal(t, len(changedFiles) > 0, tt.expected) }) } }