diff --git a/codeowners/resource_file.go b/codeowners/resource_file.go index 7b53d186..6607a7be 100644 --- a/codeowners/resource_file.go +++ b/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,8 +170,7 @@ 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 { @@ -120,7 +178,6 @@ func resourceFileCreate(d *schema.ResourceData, m interface{}) error { } 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{ diff --git a/codeowners/resource_file_test.go b/codeowners/resource_file_test.go index 0eae6970..42dab2cb 100644 --- a/codeowners/resource_file_test.go +++ b/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) diff --git a/codeowners/ruleset.go b/codeowners/ruleset.go index cfce7e9e..2efe8706 100644 --- a/codeowners/ruleset.go +++ b/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 - }