Merge branch 'master' into master

codeowners/resource_file.go

@@ -3,22 +3,24 @@ package codeowners
 import (
 	"context"
 	"fmt"
-	"github.com/form3tech-oss/go-github-utils/pkg/branch"
 	"net/http"
+	"reflect"
 	"sort"
 	"strings"
+	"sync"
 	"time"
 
-	githubcommitutils "github.com/form3tech-oss/go-github-utils/pkg/commit"
-	githubfileutils "github.com/form3tech-oss/go-github-utils/pkg/file"
 	"github.com/google/go-github/v54/github"
 	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+
+	"github.com/form3tech-oss/go-github-utils/pkg/branch"
+	githubcommitutils "github.com/form3tech-oss/go-github-utils/pkg/commit"
+	githubfileutils "github.com/form3tech-oss/go-github-utils/pkg/file"
 )
 
 const codeownersPath = ".github/CODEOWNERS"
 
 func resourceFile() *schema.Resource {
-
 	return &schema.Resource{
 		Create: resourceFileCreate,
 		Read:   resourceFileRead,
@@ -67,7 +69,8 @@ func resourceFile() *schema.Resource {
 							Elem: &schema.Schema{
 								Type: schema.TypeString,
 							},
-							Set: schema.HashString,
+							Set:              schema.HashString,
+							DiffSuppressFunc: usernamesDiffSupressFunc,
 						},
 					},
 				},
@@ -76,13 +79,69 @@ func resourceFile() *schema.Resource {
 	}
 }
 
+var diffResultCache = sync.Map{}
+
+// usernamesDiffSupressFunc ignores the "@" prefix when comparing usernames.
+// Since the DiffSuppressFunc is called for each element of a Set field we
+// cache the first result to avoid computing the same diff over and over again.
+func usernamesDiffSupressFunc(key, _, _ string, d *schema.ResourceData) bool {
+	// For a set, the key is path to the element, rather than the set
+	// e.g. "rules.0.usernames.0" so let's get the path to the set.
+	lastDotIndex := strings.LastIndex(key, ".")
+	if lastDotIndex != -1 {
+		key = string(key[:lastDotIndex])
+	}
+
+	cacheKey := fmt.Sprintf("%s.%s", d.Id(), key)
+	if cachedResult, ok := diffResultCache.Load(cacheKey); ok {
+		return cachedResult.(bool)
+	}
+
+	oldData, newData := d.GetChange(key)
+	if oldData == nil || newData == nil {
+		return false
+	}
+
+	oldSet, ok := oldData.(*schema.Set)
+	if !ok {
+		return false
+	}
+
+	newSet, ok := newData.(*schema.Set)
+	if !ok {
+		return false
+	}
+
+	oldArray := oldSet.List()
+	newArray := newSet.List()
+	if len(oldArray) != len(newArray) {
+		return false
+	}
+
+	oldItems := make([]string, len(oldArray))
+	for i, oldItem := range oldArray {
+		oldItems[i] = strings.TrimPrefix(fmt.Sprint(oldItem), "@")
+	}
+
+	newItems := make([]string, len(newArray))
+	for j, newItem := range newArray {
+		newItems[j] = strings.TrimPrefix(fmt.Sprint(newItem), "@")
+	}
+
+	sort.Strings(oldItems)
+	sort.Strings(newItems)
+
+	result := reflect.DeepEqual(oldItems, newItems)
+	diffResultCache.Store(cacheKey, result)
+	return result
+}
+
 func resourceFileImport(d *schema.ResourceData, m interface{}) ([]*schema.ResourceData, error) {
 	err := resourceFileRead(d, m)
 	return []*schema.ResourceData{d}, err
 }
 
 func resourceFileRead(d *schema.ResourceData, m interface{}) error {
-
 	config := m.(*providerConfiguration)
 
 	file := expandFile(d)
@@ -111,16 +170,14 @@ func resourceFileRead(d *schema.ResourceData, m interface{}) error {
 
 	file.Ruleset = parseRulesFile(raw)
 
-	_ = flattenFile(file, d)
-	return nil
+	return flattenFile(file, d)
 }
 
 func resourceFileCreate(d *schema.ResourceData, m interface{}) error {
 	return resourceFileCreateOrUpdate("Adding CODEOWNERS file", d, m)
 }
 
 func resourceFileCreateOrUpdate(s string, d *schema.ResourceData, m interface{}) error {
-
 	config := m.(*providerConfiguration)
 
 	file := expandFile(d)
@@ -288,7 +345,7 @@ func expandRuleset(in []interface{}) Ruleset {
 		rule := rule.(map[string]interface{})
 		var usernames []string
 		for _, username := range rule["usernames"].(*schema.Set).List() {
-			usernames = append(usernames, username.(string))
+			usernames = append(usernames, strings.TrimPrefix(username.(string), "@"))
 		}
 		sort.Strings(usernames)
 		out = append(out, Rule{

codeowners/resource_file_test.go

@@ -49,6 +49,12 @@ const testAccFileConfigUpdate = `
 		]
 	}`
 
+type testCase struct {
+	ResourceName string
+	Old          string
+	New          string
+}
+
 func TestAccResourceFile_basic(t *testing.T) {
 	var before, after File
 
@@ -107,6 +113,151 @@ func TestAccResourceFile_basic(t *testing.T) {
 	})
 }
 
+func TestAccResourceFile_OptionalAtSign(t *testing.T) {
+	leadingAtSignTestCases := []testCase{
+		{
+			ResourceName: "codeowners_file.test-add-at-sign",
+			Old: `
+	resource "codeowners_file" "test-add-at-sign" {
+		repository_name  = "enforcement-test-repo"
+		repository_owner = "form3tech-oss"
+		rules = [
+			{
+				pattern = "*"
+				usernames = [ "expert" ]
+			},
+			{
+				pattern = "*.java"
+				usernames = [ "java-expert", "java-guru" ]
+			}
+		]
+	}
+`,
+			New: `
+	resource "codeowners_file" "test-add-at-sign" {
+		repository_name  = "enforcement-test-repo"
+		repository_owner = "form3tech-oss"
+		rules = [
+			{
+				pattern = "*"
+				usernames = [ "@expert" ]
+			},
+			{
+				pattern = "*.java"
+				usernames = [ "@java-expert", "@java-guru" ]
+			}
+		]
+	}
+`,
+		},
+		{
+			ResourceName: "codeowners_file.test-remove-at-sign",
+			Old: `
+	resource "codeowners_file" "test-remove-at-sign" {
+		repository_name  = "enforcement-test-repo"
+		repository_owner = "form3tech-oss"
+		rules = [
+			{
+				pattern = "*"
+				usernames = [ "@expert" ]
+			},
+			{
+				pattern = "*.java"
+				usernames = [ "@java-expert", "@java-guru" ]
+			}
+		]
+	}
+`,
+			New: `
+	resource "codeowners_file" "test-remove-at-sign" {
+		repository_name  = "enforcement-test-repo"
+		repository_owner = "form3tech-oss"
+		rules = [
+			{
+				pattern = "*"
+				usernames = [ "expert" ]
+			},
+			{
+				pattern = "*.java"
+				usernames = [ "java-expert", "java-guru" ]
+			}
+		]
+	}
+`,
+		},
+		{
+			ResourceName: "codeowners_file.test-toggle-at-sign",
+			Old: `
+	resource "codeowners_file" "test-toggle-at-sign" {
+		repository_name  = "enforcement-test-repo"
+		repository_owner = "form3tech-oss"
+		rules = [
+			{
+				pattern = "*"
+				usernames = [ "expert" ]
+			},
+			{
+				pattern = "*.java"
+				usernames = [ "java-expert", "@java-guru" ]
+			}
+		]
+	}
+`,
+			New: `
+	resource "codeowners_file" "test-toggle-at-sign" {
+		repository_name  = "enforcement-test-repo"
+		repository_owner = "form3tech-oss"
+		rules = [
+			{
+				pattern = "*"
+				usernames = [ "@expert" ]
+			},
+			{
+				pattern = "*.java"
+				usernames = [ "@java-expert", "java-guru" ]
+			}
+		]
+	}
+`,
+		},
+	}
+
+	for _, testCase := range leadingAtSignTestCases {
+		var resFile File
+		resource.Test(t, resource.TestCase{
+			PreCheck:      func() { testAccPreCheck(t) },
+			IDRefreshName: testCase.ResourceName,
+			Providers:     testAccProviders,
+			CheckDestroy:  testAccCheckFileDestroy,
+			Steps: []resource.TestStep{
+				{
+					Config: testCase.Old,
+					Check: resource.ComposeAggregateTestCheckFunc(
+						testAccCheckFileExists(testCase.ResourceName, &resFile),
+						resource.TestCheckResourceAttr(testCase.ResourceName, "rules.#", "2"),
+						resource.TestCheckResourceAttr(testCase.ResourceName, "rules.0.pattern", "*"),
+						resource.TestCheckResourceAttr(testCase.ResourceName, "rules.0.usernames.#", "1"),
+						resource.TestCheckResourceAttr(testCase.ResourceName, "rules.1.pattern", "*.java"),
+						resource.TestCheckResourceAttr(testCase.ResourceName, "rules.1.usernames.#", "2"),
+						resource.TestCheckResourceAttr(testCase.ResourceName, "repository_name", "enforcement-test-repo"),
+						resource.TestCheckResourceAttr(testCase.ResourceName, "repository_owner", "form3tech-oss"),
+						resource.TestCheckResourceAttr(testCase.ResourceName, "branch", ""),
+					),
+				},
+				{
+					ResourceName:      testCase.ResourceName,
+					ImportState:       true,
+					ImportStateVerify: true,
+				},
+				{
+					Config:   testCase.New,
+					PlanOnly: true,
+				},
+			},
+		})
+	}
+}
+
 func testAccCheckFileDestroy(s *terraform.State) error {
 	config := testAccProvider.Meta().(*providerConfiguration)
 

codeowners/ruleset.go

@@ -25,21 +25,20 @@ func (ruleset Ruleset) Compile() []byte {
 	}
 	output := "# automatically generated by terraform - please do not edit here\n"
 	for _, rule := range ruleset {
-		usernames := ""
+		usernames := []string{}
 		for _, username := range rule.Usernames {
-			if !strings.Contains(username, "@") {
-				usernames = fmt.Sprintf("%s@%s ", usernames, username)
-			} else {
-				usernames = fmt.Sprintf("%s%s ", usernames, username)
+			if !strings.HasPrefix(username, "@") {
+				username = "@" + username
 			}
+
+			usernames = append(usernames, username)
 		}
-		output = fmt.Sprintf("%s%s %s\n", output, rule.Pattern, usernames)
+		output = fmt.Sprintf("%s%s %s\n", output, rule.Pattern, strings.Join(usernames, " "))
 	}
 	return []byte(output)
 }
 
 func parseRulesFile(data string) Ruleset {
-
 	var rules []Rule
 	lines := strings.Split(data, "\n")
 	for _, line := range lines {
@@ -61,14 +60,11 @@ func parseRulesFile(data string) Ruleset {
 			if len(username) == 0 { // may be split by multiple spaces
 				continue
 			}
-			if username[0] == '@' {
-				username = username[1:]
-			}
-			rule.Usernames = append(rule.Usernames, username)
+
+			rule.Usernames = append(rule.Usernames, strings.TrimPrefix(username, "@"))
 		}
 		rules = append(rules, rule)
 	}
 
 	return rules
-
 }