From 42e63fbc3512205232aac1fd4ae0a8db1f5092c7 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Thu, 22 Jan 2026 22:34:22 +0200 Subject: [PATCH 1/6] Remove duplicate code Signed-off-by: Timo Sand --- github/resource_github_repository.go | 12 +++------- github/resource_github_repository_test.go | 27 +++++++++++++++-------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/github/resource_github_repository.go b/github/resource_github_repository.go index fca6da113d..32a824cba3 100644 --- a/github/resource_github_repository.go +++ b/github/resource_github_repository.go @@ -991,19 +991,13 @@ func resourceGithubRepositoryUpdate(ctx context.Context, d *schema.ResourceData, if d.HasChange("topics") { topics := repoReq.Topics - _, _, err = client.Repositories.ReplaceAllTopics(ctx, owner, *repo.Name, topics) + // GitHub API docs say that the ReplaceAllTopics endpoint should be used instead of updating the repository with the topics field + // https://docs.github.com/en/rest/repos/repos?apiVersion=2022-11-28#update-a-repository + _, _, err := client.Repositories.ReplaceAllTopics(ctx, owner, *repo.Name, topics) if err != nil { return diag.FromErr(err) } d.SetId(*repo.Name) - - if d.HasChange("topics") { - topics := repoReq.Topics - _, _, err = client.Repositories.ReplaceAllTopics(ctx, owner, *repo.Name, topics) - if err != nil { - return diag.FromErr(err) - } - } } if d.IsNewResource() || d.HasChange("vulnerability_alerts") { diff --git a/github/resource_github_repository_test.go b/github/resource_github_repository_test.go index 1e939edd44..1cf6e93a30 100644 --- a/github/resource_github_repository_test.go +++ b/github/resource_github_repository_test.go @@ -369,7 +369,7 @@ resource "github_repository" "test" { }) }) - t.Run("configures topics for a repository", func(t *testing.T) { + t.Run("configures_topics_for_a_repository", func(t *testing.T) { randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) testRepoName := fmt.Sprintf("%stopic-%s", testResourcePrefix, randomID) config := fmt.Sprintf(` @@ -380,20 +380,29 @@ resource "github_repository" "test" { } `, testRepoName) - check := resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr( - "github_repository.test", "topics.#", - "2", - ), - ) - resource.Test(t, resource.TestCase{ PreCheck: func() { skipUnauthenticated(t) }, ProviderFactories: providerFactories, Steps: []resource.TestStep{ { Config: config, - Check: check, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "github_repository.test", "topics.#", + "2", + ), + ), + }, + { + Config: strings.Replace(config, + `topics = ["terraform", "testing"]`, + `topics = ["terraform", "testing", "extra-topic"]`, 1), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "github_repository.test", "topics.#", + "3", + ), + ), }, }, }) From c2982d01e1118e98be9fd8bc05396304e984935a Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Tue, 27 Jan 2026 22:28:19 +0200 Subject: [PATCH 2/6] Inline `repoReq.Topics` Signed-off-by: Timo Sand --- github/resource_github_repository.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/github/resource_github_repository.go b/github/resource_github_repository.go index 32a824cba3..071d6b4e01 100644 --- a/github/resource_github_repository.go +++ b/github/resource_github_repository.go @@ -990,10 +990,9 @@ func resourceGithubRepositoryUpdate(ctx context.Context, d *schema.ResourceData, } if d.HasChange("topics") { - topics := repoReq.Topics // GitHub API docs say that the ReplaceAllTopics endpoint should be used instead of updating the repository with the topics field // https://docs.github.com/en/rest/repos/repos?apiVersion=2022-11-28#update-a-repository - _, _, err := client.Repositories.ReplaceAllTopics(ctx, owner, *repo.Name, topics) + _, _, err := client.Repositories.ReplaceAllTopics(ctx, owner, repo.GetName(), repoReq.Topics) if err != nil { return diag.FromErr(err) } From 2df4f335c527136d15726cad9831a533131ad033 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Tue, 27 Jan 2026 22:49:43 +0200 Subject: [PATCH 3/6] Remove clearly unnecessary `SetId` Signed-off-by: Timo Sand --- github/resource_github_repository.go | 1 - 1 file changed, 1 deletion(-) diff --git a/github/resource_github_repository.go b/github/resource_github_repository.go index 071d6b4e01..c98dd0c15b 100644 --- a/github/resource_github_repository.go +++ b/github/resource_github_repository.go @@ -996,7 +996,6 @@ func resourceGithubRepositoryUpdate(ctx context.Context, d *schema.ResourceData, if err != nil { return diag.FromErr(err) } - d.SetId(*repo.Name) } if d.IsNewResource() || d.HasChange("vulnerability_alerts") { From f4349542eca936df318c563b50c34d90c1696893 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Tue, 27 Jan 2026 22:50:31 +0200 Subject: [PATCH 4/6] Use `repo.GetName()` instead of `*repo.Name` Signed-off-by: Timo Sand --- github/resource_github_repository.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/github/resource_github_repository.go b/github/resource_github_repository.go index c98dd0c15b..ffbf2bdcaf 100644 --- a/github/resource_github_repository.go +++ b/github/resource_github_repository.go @@ -683,7 +683,7 @@ func resourceGithubRepositoryCreate(ctx context.Context, d *schema.ResourceData, return diag.FromErr(err) } - d.SetId(*repo.Name) + d.SetId(repo.GetName()) } } else if d.Get("fork").(string) == "true" { // Handle repository forking @@ -963,7 +963,7 @@ func resourceGithubRepositoryUpdate(ctx context.Context, d *schema.ResourceData, if err != nil { return diag.FromErr(err) } - d.SetId(*repo.Name) + d.SetId(repo.GetName()) // It's possible that `repo.GetName()` is different from `repoName` if the repository is renamed if d.HasChange("pages") && !d.IsNewResource() { opts := expandPagesUpdate(d.Get("pages").([]any)) From b491e9e5dbe91e5aa3b4804582f3ca2377f15da3 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Tue, 27 Jan 2026 23:05:41 +0200 Subject: [PATCH 5/6] Address review comments Signed-off-by: Timo Sand --- github/resource_github_repository_test.go | 24 +++++++++-------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/github/resource_github_repository_test.go b/github/resource_github_repository_test.go index 1cf6e93a30..a5c18b31dc 100644 --- a/github/resource_github_repository_test.go +++ b/github/resource_github_repository_test.go @@ -372,36 +372,30 @@ resource "github_repository" "test" { t.Run("configures_topics_for_a_repository", func(t *testing.T) { randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) testRepoName := fmt.Sprintf("%stopic-%s", testResourcePrefix, randomID) - config := fmt.Sprintf(` + topicsBefore := `["terraform", "testing"]` + topicsAfter := `["terraform", "testing", "extra-topic"]` + config := ` resource "github_repository" "test" { name = "%s" description = "Terraform acceptance tests %[1]s" - topics = ["terraform", "testing"] + topics = %s } - `, testRepoName) + ` resource.Test(t, resource.TestCase{ PreCheck: func() { skipUnauthenticated(t) }, ProviderFactories: providerFactories, Steps: []resource.TestStep{ { - Config: config, + Config: fmt.Sprintf(config, testRepoName, topicsBefore), Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr( - "github_repository.test", "topics.#", - "2", - ), + resource.TestCheckResourceAttr("github_repository.test", "topics.#", "2"), ), }, { - Config: strings.Replace(config, - `topics = ["terraform", "testing"]`, - `topics = ["terraform", "testing", "extra-topic"]`, 1), + Config: fmt.Sprintf(config, testRepoName, topicsAfter), Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr( - "github_repository.test", "topics.#", - "3", - ), + resource.TestCheckResourceAttr("github_repository.test", "topics.#", "3"), ), }, }, From fe1294f7451e6af6d54ef8935b181e7b03b09d99 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Tue, 27 Jan 2026 23:06:35 +0200 Subject: [PATCH 6/6] Some test structure reformatting Signed-off-by: Timo Sand --- github/resource_github_repository_test.go | 75 ++++++----------------- 1 file changed, 18 insertions(+), 57 deletions(-) diff --git a/github/resource_github_repository_test.go b/github/resource_github_repository_test.go index a5c18b31dc..8f3152a35b 100644 --- a/github/resource_github_repository_test.go +++ b/github/resource_github_repository_test.go @@ -418,20 +418,15 @@ resource "github_repository" "test" { } `, testRepoName, testAccConf.testPublicTemplateRepositoryOwner, testAccConf.testPublicTemplateRepository) - check := resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr( - "github_repository.test", "is_template", - "false", - ), - ) - resource.Test(t, resource.TestCase{ PreCheck: func() { skipUnauthenticated(t) }, ProviderFactories: providerFactories, Steps: []resource.TestStep{ { Config: config, - Check: check, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_repository.test", "is_template", "false"), + ), }, }, }) @@ -453,20 +448,15 @@ resource "github_repository" "test" { } `, testRepoName, testAccConf.owner, testAccConf.testOrgTemplateRepository) - check := resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr( - "github_repository.test", "is_template", - "false", - ), - ) - resource.Test(t, resource.TestCase{ PreCheck: func() { skipUnlessHasOrgs(t) }, ProviderFactories: providerFactories, Steps: []resource.TestStep{ { Config: config, - Check: check, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_repository.test", "is_template", "false"), + ), }, }, }) @@ -791,40 +781,23 @@ resource "github_repository" "test" { `, testRepoName, updatedMergeCommitTitle, updatedMergeCommitMessage), } - checks := map[string]resource.TestCheckFunc{ - "before": resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr( - "github_repository.test", "merge_commit_title", - mergeCommitTitle, - ), - resource.TestCheckResourceAttr( - "github_repository.test", "merge_commit_message", - mergeCommitMessage, - ), - ), - "after": resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr( - "github_repository.test", "merge_commit_title", - updatedMergeCommitTitle, - ), - resource.TestCheckResourceAttr( - "github_repository.test", "merge_commit_message", - updatedMergeCommitMessage, - ), - ), - } - resource.Test(t, resource.TestCase{ PreCheck: func() { skipUnauthenticated(t) }, ProviderFactories: providerFactories, Steps: []resource.TestStep{ { Config: configs["before"], - Check: checks["before"], + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_repository.test", "merge_commit_title", mergeCommitTitle), + resource.TestCheckResourceAttr("github_repository.test", "merge_commit_message", mergeCommitMessage), + ), }, { Config: configs["after"], - Check: checks["after"], + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_repository.test", "merge_commit_title", updatedMergeCommitTitle), + resource.TestCheckResourceAttr("github_repository.test", "merge_commit_message", updatedMergeCommitMessage), + ), }, }, }) @@ -860,24 +833,12 @@ resource "github_repository" "test" { checks := map[string]resource.TestCheckFunc{ "before": resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr( - "github_repository.test", "squash_merge_commit_title", - squashMergeCommitTitle, - ), - resource.TestCheckResourceAttr( - "github_repository.test", "squash_merge_commit_message", - squashMergeCommitMessage, - ), + resource.TestCheckResourceAttr("github_repository.test", "squash_merge_commit_title", squashMergeCommitTitle), + resource.TestCheckResourceAttr("github_repository.test", "squash_merge_commit_message", squashMergeCommitMessage), ), "after": resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr( - "github_repository.test", "squash_merge_commit_title", - updatedSquashMergeCommitTitle, - ), - resource.TestCheckResourceAttr( - "github_repository.test", "squash_merge_commit_message", - updatedSquashMergeCommitMessage, - ), + resource.TestCheckResourceAttr("github_repository.test", "squash_merge_commit_title", updatedSquashMergeCommitTitle), + resource.TestCheckResourceAttr("github_repository.test", "squash_merge_commit_message", updatedSquashMergeCommitMessage), ), }