Remove replace directive labels when not needed (#117)

Previously we didn't remove labels when a replace directive was removed. This would mean replace directive labels would likely grow where they should be managed automatically by puku. This PR removes any replace directive labels on targets synced which no longer have replace directives in the go.mod.

edit/edit.go

@@ -179,3 +179,37 @@ func AddLabel(rule *build.Rule, label string) error {
 	rule.SetAttr("labels", ruleLabelsList)
 	return nil
 }
+
+func RemoveLabel(rule *build.Rule, label string) error {
+	// Fetch the labels attribute, if it exists
+	ruleLabels := rule.Attr("labels")
+	if ruleLabels == nil {
+		return nil
+	}
+	// Check it's a list of expressions
+	ruleLabelsList, ok := ruleLabels.(*build.ListExpr)
+	if !ok {
+		return errors.New("rule already has a `labels` attribute, and it is not a list")
+	}
+	// Build a new list of labels which doesn't include the specified one.
+	newLabels := &build.ListExpr{}
+	for _, labelExpr := range ruleLabelsList.List {
+		// Copy any non-string labels
+		labelStringExpr, ok := labelExpr.(*build.StringExpr)
+		if !ok {
+			newLabels.List = append(newLabels.List, labelExpr)
+			continue
+		}
+		// Copy any string labels which do not match
+		if labelStringExpr.Value != label {
+			newLabels.List = append(newLabels.List, labelExpr)
+		}
+	}
+	// If labels is empty, remove the attribute
+	if len(newLabels.List) == 0 {
+		rule.DelAttr("labels")
+	} else {
+		rule.SetAttr("labels", newLabels)
+	}
+	return nil
+}

sync/integration/syncmod/sync_mod_test.go

@@ -42,7 +42,7 @@ func TestModSync(t *testing.T) {
 	// Read version info from the go.mod file
 	expectedVers := readModFileVersions()
 
-	// We expect to generate the following for the replace in the go.mod:
+	// We expect to generate the following for the replace directives in the go.mod:
 	// go_mod_download(
 	//   name = "github.com_peterebden_buildtools_dl",
 	//   module = "github.com/peterebden/buildtools",
@@ -52,6 +52,13 @@ func TestModSync(t *testing.T) {
 	// go_repo(
 	//   download = ":github.com_peterebden_buildtools_dl",
 	//   module = "github.com/bazelbuild/buildtools",
+	//   labels = ["go_replace_directive"],
+	// )
+	//
+	// go_repo(
+	//   module = "github.com/stretchr/testify",
+	//   version = "v1.3.0",
+	//   labels = ["go_replace_directive"],
 	// )
 
 	for _, repoRule := range thirdPartyBuildFile.Rules("go_repo") {
@@ -68,10 +75,16 @@ func TestModSync(t *testing.T) {
 				return
 			}
 
-			// Check that testify is labelled for a replace directive
+			// Check that testify is the only other one labelled for a replace directive
+			labels := listLabels(repoRule)
 			if repoRule.AttrString("module") == "github.com/stretchr/testify" {
-				labels := listLabels(repoRule)
 				assert.Contains(t, labels, "go_replace_directive")
+			} else {
+				assert.NotContains(t, labels, "go_replace_directive")
+				// Ensure there aren't empty labels list attributes
+				if len(labels) == 0 {
+					assert.Nil(t, repoRule.Attr("labels"))
+				}
 			}
 
 			// All rules start off at v0.0.1 and should be updated to their version listed in the go.mod
@@ -90,6 +103,9 @@ func TestModSync(t *testing.T) {
 
 func listLabels(rule *build.Rule) []string {
 	labelsExpr := rule.Attr("labels")
+	if labelsExpr == nil {
+		return []string{}
+	}
 	labels := make([]string, 0, len(labelsExpr.(*build.ListExpr).List))
 	for _, labelExpr := range labelsExpr.(*build.ListExpr).List {
 		labels = append(labels, labelExpr.(*build.StringExpr).Value)

sync/integration/syncmod/test_repo/third_party/go/BUILD_FILE

@@ -6,16 +6,22 @@ go_repo(
 )
 
 go_repo(
+    labels = [
+      "example_label",
+      "go_replace_directive",
+    ],
     module = "github.com/davecgh/go-spew",
     version = "v0.0.1",
 )
 
 go_repo(
+    labels = ["example_label"],
     module = "github.com/pmezard/go-difflib",
     version = "v0.0.1",
 )
 
 go_repo(
+    labels = ["go_replace_directive"],
     module = "github.com/stretchr/objx",
     version = "v0.0.1",
 )

sync/sync.go

@@ -103,13 +103,32 @@ func (s *syncer) syncModFile(conf *config.Config, file *build.File, existingRule
 		return err
 	}
 
+	// Remove "go_replace_directive" label from any rules which lack a replace directive
+	for modPath, rule := range existingRules {
+		// Find any matching replace directive
+		var matchingReplace *modfile.Replace
+		for _, replace := range f.Replace {
+			if replace.Old.Path == modPath {
+				matchingReplace = replace
+			}
+		}
+
+		// Remove the replace label if not needed
+		if matchingReplace == nil {
+			err := edit.RemoveLabel(rule, ReplaceLabel)
+			if err != nil {
+				log.Warningf("Failed to remove replace label from %v: %v", modPath, err)
+			}
+		}
+	}
+
+	// Check all modules listed in go.mod
 	for _, req := range f.Require {
-		reqVersion := req.Mod.Version
+		// Find any matching replace directive
 		var matchingReplace *modfile.Replace
-		for _, r := range f.Replace {
-			if r.Old.Path == req.Mod.Path {
-				matchingReplace = r
-				reqVersion = r.New.Version
+		for _, replace := range f.Replace {
+			if replace.Old.Path == req.Mod.Path {
+				matchingReplace = replace
 			}
 		}
 
@@ -121,36 +140,58 @@ func (s *syncer) syncModFile(conf *config.Config, file *build.File, existingRule
 				// delete the old go_repo rule and regenerate it with a go_mod_download and a go_repo.
 				edit.RemoveTarget(file, rule)
 			} else {
-				// Make sure the version is up-to-date
-				rule.SetAttr("version", edit.NewStringExpr(reqVersion))
-				// Add label for the replace directive
-				if matchingReplace != nil {
-					err := edit.AddLabel(rule, ReplaceLabel)
-					if err != nil {
-						log.Warningf("Failed to add replace label to %v: %v", req.Mod.Path, err)
-					}
-				}
+				s.syncExistingRule(rule, req, matchingReplace)
 				// No other changes needed
 				continue
 			}
 		}
 
-		ls, err := s.licences.Get(req.Mod.Path, req.Mod.Version)
-		if err != nil {
-			return fmt.Errorf("failed to get licences for %v: %v", req.Mod.Path, err)
+		// Add a new rule to the build file if one does not exist
+		if err = s.addNewRule(file, req, matchingReplace); err != nil {
+			return fmt.Errorf("failed to add new rule %v: %v", req.Mod.Path, err)
 		}
+	}
+
+	return nil
+}
 
-		// If no replace directive, or replace directive is just replacing the version, add a simple rule
-		if matchingReplace == nil || matchingReplace.New.Path == req.Mod.Path {
-			file.Stmt = append(file.Stmt, edit.NewGoRepoRule(req.Mod.Path, reqVersion, "", ls, []string{ReplaceLabel}))
-			continue
+func (s *syncer) syncExistingRule(rule *build.Rule, requireDirective *modfile.Require, replaceDirective *modfile.Replace) {
+	reqVersion := requireDirective.Mod.Version
+	// Add label for the replace directive
+	if replaceDirective != nil {
+		err := edit.AddLabel(rule, ReplaceLabel)
+		if err != nil {
+			log.Warningf("Failed to add replace label to %v: %v", requireDirective.Mod.Path, err)
 		}
+		// Update the requested version
+		reqVersion = replaceDirective.New.Version
+	}
+	// Make sure the version is up-to-date
+	rule.SetAttr("version", edit.NewStringExpr(reqVersion))
+}
+
+func (s *syncer) addNewRule(file *build.File, requireDirective *modfile.Require, replaceDirective *modfile.Replace) error {
+	// List licences
+	ls, err := s.licences.Get(requireDirective.Mod.Path, requireDirective.Mod.Version)
+	if err != nil {
+		return fmt.Errorf("failed to get licences for %v: %v", requireDirective.Mod.Path, err)
+	}
+
+	// If no replace directive, add a simple rule
+	if replaceDirective == nil {
+		file.Stmt = append(file.Stmt, edit.NewGoRepoRule(requireDirective.Mod.Path, requireDirective.Mod.Version, "", ls, []string{}))
+		return nil
+	}
 
-		dl, dlName := edit.NewModDownloadRule(matchingReplace.New.Path, matchingReplace.New.Version, ls)
-		file.Stmt = append(file.Stmt, dl)
-		file.Stmt = append(file.Stmt, edit.NewGoRepoRule(req.Mod.Path, "", dlName, nil, []string{ReplaceLabel}))
+	// If replace directive is just replacing the version, add a simple rule
+	if replaceDirective.New.Path == requireDirective.Mod.Path {
+		file.Stmt = append(file.Stmt, edit.NewGoRepoRule(requireDirective.Mod.Path, replaceDirective.New.Version, "", ls, []string{ReplaceLabel}))
+		return nil
 	}
 
+	dl, dlName := edit.NewModDownloadRule(replaceDirective.New.Path, replaceDirective.New.Version, ls)
+	file.Stmt = append(file.Stmt, dl)
+	file.Stmt = append(file.Stmt, edit.NewGoRepoRule(requireDirective.Mod.Path, "", dlName, nil, []string{ReplaceLabel}))
 	return nil
 }