Skip to content

Commit 272856f

Browse files
deigadeiga
authored
[MAINT] Remove unnecessary separate API call for repo topics in github_repository (#3086)
* Remove duplicate code Signed-off-by: Timo Sand <[email protected]> * Inline `repoReq.Topics` Signed-off-by: Timo Sand <[email protected]> * Remove clearly unnecessary `SetId` Signed-off-by: Timo Sand <[email protected]> * Use `repo.GetName()` instead of `*repo.Name` Signed-off-by: Timo Sand <[email protected]> * Address review comments Signed-off-by: Timo Sand <[email protected]> * Some test structure reformatting Signed-off-by: Timo Sand <[email protected]> --------- Signed-off-by: Timo Sand <[email protected]>
1 parent 06af0a6 commit 272856f

2 files changed

Lines changed: 39 additions & 83 deletions

File tree

‎github/resource_github_repository.go‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -683,7 +683,7 @@ func resourceGithubRepositoryCreate(ctx context.Context, d *schema.ResourceData,
683683
return diag.FromErr(err)
684684
}
685685

686-
d.SetId(*repo.Name)
686+
d.SetId(repo.GetName())
687687
}
688688
} else if d.Get("fork").(string) == "true" {
689689
// Handle repository forking
@@ -963,7 +963,7 @@ func resourceGithubRepositoryUpdate(ctx context.Context, d *schema.ResourceData,
963963
if err != nil {
964964
return diag.FromErr(err)
965965
}
966-
d.SetId(*repo.Name)
966+
d.SetId(repo.GetName()) // It's possible that `repo.GetName()` is different from `repoName` if the repository is renamed
967967

968968
if d.HasChange("pages") && !d.IsNewResource() {
969969
opts := expandPagesUpdate(d.Get("pages").([]any))
@@ -990,20 +990,12 @@ func resourceGithubRepositoryUpdate(ctx context.Context, d *schema.ResourceData,
990990
}
991991

992992
if d.HasChange("topics") {
993-
topics := repoReq.Topics
994-
_, _, err = client.Repositories.ReplaceAllTopics(ctx, owner, *repo.Name, topics)
993+
// GitHub API docs say that the ReplaceAllTopics endpoint should be used instead of updating the repository with the topics field
994+
// https://docs.github.com/en/rest/repos/repos?apiVersion=2022-11-28#update-a-repository
995+
_, _, err := client.Repositories.ReplaceAllTopics(ctx, owner, repo.GetName(), repoReq.Topics)
995996
if err != nil {
996997
return diag.FromErr(err)
997998
}
998-
d.SetId(*repo.Name)
999-
1000-
if d.HasChange("topics") {
1001-
topics := repoReq.Topics
1002-
_, _, err = client.Repositories.ReplaceAllTopics(ctx, owner, *repo.Name, topics)
1003-
if err != nil {
1004-
return diag.FromErr(err)
1005-
}
1006-
}
1007999
}
10081000

10091001
if d.IsNewResource() || d.HasChange("vulnerability_alerts") {

‎github/resource_github_repository_test.go‎

Lines changed: 34 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -369,31 +369,34 @@ resource "github_repository" "test" {
369369
})
370370
})
371371

372-
t.Run("configures topics for a repository", func(t *testing.T) {
372+
t.Run("configures_topics_for_a_repository", func(t *testing.T) {
373373
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
374374
testRepoName := fmt.Sprintf("%stopic-%s", testResourcePrefix, randomID)
375-
config := fmt.Sprintf(`
375+
topicsBefore := `["terraform", "testing"]`
376+
topicsAfter := `["terraform", "testing", "extra-topic"]`
377+
config := `
376378
resource "github_repository" "test" {
377379
name = "%s"
378380
description = "Terraform acceptance tests %[1]s"
379-
topics = ["terraform", "testing"]
381+
topics = %s
380382
}
381-
`, testRepoName)
382-
383-
check := resource.ComposeTestCheckFunc(
384-
resource.TestCheckResourceAttr(
385-
"github_repository.test", "topics.#",
386-
"2",
387-
),
388-
)
383+
`
389384

390385
resource.Test(t, resource.TestCase{
391386
PreCheck: func() { skipUnauthenticated(t) },
392387
ProviderFactories: providerFactories,
393388
Steps: []resource.TestStep{
394389
{
395-
Config: config,
396-
Check: check,
390+
Config: fmt.Sprintf(config, testRepoName, topicsBefore),
391+
Check: resource.ComposeTestCheckFunc(
392+
resource.TestCheckResourceAttr("github_repository.test", "topics.#", "2"),
393+
),
394+
},
395+
{
396+
Config: fmt.Sprintf(config, testRepoName, topicsAfter),
397+
Check: resource.ComposeTestCheckFunc(
398+
resource.TestCheckResourceAttr("github_repository.test", "topics.#", "3"),
399+
),
397400
},
398401
},
399402
})
@@ -415,20 +418,15 @@ resource "github_repository" "test" {
415418
}
416419
`, testRepoName, testAccConf.testPublicTemplateRepositoryOwner, testAccConf.testPublicTemplateRepository)
417420

418-
check := resource.ComposeTestCheckFunc(
419-
resource.TestCheckResourceAttr(
420-
"github_repository.test", "is_template",
421-
"false",
422-
),
423-
)
424-
425421
resource.Test(t, resource.TestCase{
426422
PreCheck: func() { skipUnauthenticated(t) },
427423
ProviderFactories: providerFactories,
428424
Steps: []resource.TestStep{
429425
{
430426
Config: config,
431-
Check: check,
427+
Check: resource.ComposeTestCheckFunc(
428+
resource.TestCheckResourceAttr("github_repository.test", "is_template", "false"),
429+
),
432430
},
433431
},
434432
})
@@ -450,20 +448,15 @@ resource "github_repository" "test" {
450448
}
451449
`, testRepoName, testAccConf.owner, testAccConf.testOrgTemplateRepository)
452450

453-
check := resource.ComposeTestCheckFunc(
454-
resource.TestCheckResourceAttr(
455-
"github_repository.test", "is_template",
456-
"false",
457-
),
458-
)
459-
460451
resource.Test(t, resource.TestCase{
461452
PreCheck: func() { skipUnlessHasOrgs(t) },
462453
ProviderFactories: providerFactories,
463454
Steps: []resource.TestStep{
464455
{
465456
Config: config,
466-
Check: check,
457+
Check: resource.ComposeTestCheckFunc(
458+
resource.TestCheckResourceAttr("github_repository.test", "is_template", "false"),
459+
),
467460
},
468461
},
469462
})
@@ -788,40 +781,23 @@ resource "github_repository" "test" {
788781
`, testRepoName, updatedMergeCommitTitle, updatedMergeCommitMessage),
789782
}
790783

791-
checks := map[string]resource.TestCheckFunc{
792-
"before": resource.ComposeTestCheckFunc(
793-
resource.TestCheckResourceAttr(
794-
"github_repository.test", "merge_commit_title",
795-
mergeCommitTitle,
796-
),
797-
resource.TestCheckResourceAttr(
798-
"github_repository.test", "merge_commit_message",
799-
mergeCommitMessage,
800-
),
801-
),
802-
"after": resource.ComposeTestCheckFunc(
803-
resource.TestCheckResourceAttr(
804-
"github_repository.test", "merge_commit_title",
805-
updatedMergeCommitTitle,
806-
),
807-
resource.TestCheckResourceAttr(
808-
"github_repository.test", "merge_commit_message",
809-
updatedMergeCommitMessage,
810-
),
811-
),
812-
}
813-
814784
resource.Test(t, resource.TestCase{
815785
PreCheck: func() { skipUnauthenticated(t) },
816786
ProviderFactories: providerFactories,
817787
Steps: []resource.TestStep{
818788
{
819789
Config: configs["before"],
820-
Check: checks["before"],
790+
Check: resource.ComposeTestCheckFunc(
791+
resource.TestCheckResourceAttr("github_repository.test", "merge_commit_title", mergeCommitTitle),
792+
resource.TestCheckResourceAttr("github_repository.test", "merge_commit_message", mergeCommitMessage),
793+
),
821794
},
822795
{
823796
Config: configs["after"],
824-
Check: checks["after"],
797+
Check: resource.ComposeTestCheckFunc(
798+
resource.TestCheckResourceAttr("github_repository.test", "merge_commit_title", updatedMergeCommitTitle),
799+
resource.TestCheckResourceAttr("github_repository.test", "merge_commit_message", updatedMergeCommitMessage),
800+
),
825801
},
826802
},
827803
})
@@ -857,24 +833,12 @@ resource "github_repository" "test" {
857833

858834
checks := map[string]resource.TestCheckFunc{
859835
"before": resource.ComposeTestCheckFunc(
860-
resource.TestCheckResourceAttr(
861-
"github_repository.test", "squash_merge_commit_title",
862-
squashMergeCommitTitle,
863-
),
864-
resource.TestCheckResourceAttr(
865-
"github_repository.test", "squash_merge_commit_message",
866-
squashMergeCommitMessage,
867-
),
836+
resource.TestCheckResourceAttr("github_repository.test", "squash_merge_commit_title", squashMergeCommitTitle),
837+
resource.TestCheckResourceAttr("github_repository.test", "squash_merge_commit_message", squashMergeCommitMessage),
868838
),
869839
"after": resource.ComposeTestCheckFunc(
870-
resource.TestCheckResourceAttr(
871-
"github_repository.test", "squash_merge_commit_title",
872-
updatedSquashMergeCommitTitle,
873-
),
874-
resource.TestCheckResourceAttr(
875-
"github_repository.test", "squash_merge_commit_message",
876-
updatedSquashMergeCommitMessage,
877-
),
840+
resource.TestCheckResourceAttr("github_repository.test", "squash_merge_commit_title", updatedSquashMergeCommitTitle),
841+
resource.TestCheckResourceAttr("github_repository.test", "squash_merge_commit_message", updatedSquashMergeCommitMessage),
878842
),
879843
}
880844

0 commit comments

Comments
 (0)