[multimod] apply review feedback

The following changes were applied:
- remove unnecessary bool return
- improve documentation
- move Client interface

Signed-off-by: Alex Boten <aboten@lightstep.com>

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

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)

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
-}

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
 }

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