Skip to content

Commit 19a8ecc

Browse files
steveteubersteveteuber
authored
fix: only set web_commit_signoff_required if explicitly configured (#3165)
* fix: only set web_commit_signoff_required if explicitly configured * fix: set web commit signoff only for new resources and changes * test: check web commit signoff required not set * fix: update commit signoff settings and test * test: split test into 3 tests for the 3 states
1 parent dd3f1c2 commit 19a8ecc

File tree

3 files changed

+139
-42
lines changed

3 files changed

+139
-42
lines changed

‎github/resource_github_repository.go‎

Lines changed: 33 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -287,8 +287,8 @@ func resourceGithubRepository() *schema.Resource {
287287
"web_commit_signoff_required": {
288288
Type: schema.TypeBool,
289289
Optional: true,
290-
Default: false,
291-
Description: "Require contributors to sign off on web-based commits. Defaults to 'false'.",
290+
Computed: true,
291+
Description: "Require contributors to sign off on web-based commits.",
292292
},
293293
"auto_init": {
294294
Type: schema.TypeBool,
@@ -586,29 +586,28 @@ func resourceGithubRepositoryObject(d *schema.ResourceData) *github.Repository {
586586
visibility := calculateVisibility(d)
587587

588588
repository := &github.Repository{
589-
Name: github.Ptr(d.Get("name").(string)),
590-
Description: github.Ptr(d.Get("description").(string)),
591-
Homepage: github.Ptr(d.Get("homepage_url").(string)),
592-
Visibility: github.Ptr(visibility),
593-
HasDownloads: github.Ptr(d.Get("has_downloads").(bool)),
594-
HasIssues: github.Ptr(d.Get("has_issues").(bool)),
595-
HasDiscussions: github.Ptr(d.Get("has_discussions").(bool)),
596-
HasProjects: github.Ptr(d.Get("has_projects").(bool)),
597-
HasWiki: github.Ptr(d.Get("has_wiki").(bool)),
598-
IsTemplate: github.Ptr(d.Get("is_template").(bool)),
599-
AllowMergeCommit: github.Ptr(d.Get("allow_merge_commit").(bool)),
600-
AllowSquashMerge: github.Ptr(d.Get("allow_squash_merge").(bool)),
601-
AllowRebaseMerge: github.Ptr(d.Get("allow_rebase_merge").(bool)),
602-
AllowAutoMerge: github.Ptr(d.Get("allow_auto_merge").(bool)),
603-
DeleteBranchOnMerge: github.Ptr(d.Get("delete_branch_on_merge").(bool)),
604-
WebCommitSignoffRequired: github.Ptr(d.Get("web_commit_signoff_required").(bool)),
605-
AutoInit: github.Ptr(d.Get("auto_init").(bool)),
606-
LicenseTemplate: github.Ptr(d.Get("license_template").(string)),
607-
GitignoreTemplate: github.Ptr(d.Get("gitignore_template").(string)),
608-
Archived: github.Ptr(d.Get("archived").(bool)),
609-
Topics: expandStringList(d.Get("topics").(*schema.Set).List()),
610-
AllowUpdateBranch: github.Ptr(d.Get("allow_update_branch").(bool)),
611-
SecurityAndAnalysis: calculateSecurityAndAnalysis(d),
589+
Name: github.Ptr(d.Get("name").(string)),
590+
Description: github.Ptr(d.Get("description").(string)),
591+
Homepage: github.Ptr(d.Get("homepage_url").(string)),
592+
Visibility: github.Ptr(visibility),
593+
HasDownloads: github.Ptr(d.Get("has_downloads").(bool)),
594+
HasIssues: github.Ptr(d.Get("has_issues").(bool)),
595+
HasDiscussions: github.Ptr(d.Get("has_discussions").(bool)),
596+
HasProjects: github.Ptr(d.Get("has_projects").(bool)),
597+
HasWiki: github.Ptr(d.Get("has_wiki").(bool)),
598+
IsTemplate: github.Ptr(d.Get("is_template").(bool)),
599+
AllowMergeCommit: github.Ptr(d.Get("allow_merge_commit").(bool)),
600+
AllowSquashMerge: github.Ptr(d.Get("allow_squash_merge").(bool)),
601+
AllowRebaseMerge: github.Ptr(d.Get("allow_rebase_merge").(bool)),
602+
AllowAutoMerge: github.Ptr(d.Get("allow_auto_merge").(bool)),
603+
DeleteBranchOnMerge: github.Ptr(d.Get("delete_branch_on_merge").(bool)),
604+
AutoInit: github.Ptr(d.Get("auto_init").(bool)),
605+
LicenseTemplate: github.Ptr(d.Get("license_template").(string)),
606+
GitignoreTemplate: github.Ptr(d.Get("gitignore_template").(string)),
607+
Archived: github.Ptr(d.Get("archived").(bool)),
608+
Topics: expandStringList(d.Get("topics").(*schema.Set).List()),
609+
AllowUpdateBranch: github.Ptr(d.Get("allow_update_branch").(bool)),
610+
SecurityAndAnalysis: calculateSecurityAndAnalysis(d),
612611
}
613612

614613
// only configure merge commit if we are in commit merge strategy
@@ -638,6 +637,15 @@ func resourceGithubRepositoryObject(d *schema.ResourceData) *github.Repository {
638637
}
639638
}
640639

640+
// only configure web commit signoff if explicitly set in the configuration
641+
if d.IsNewResource() || d.HasChange("web_commit_signoff_required") {
642+
if webCommitSignoffRequired, ok := d.GetOkExists("web_commit_signoff_required"); ok { //nolint:staticcheck,SA1019 // We sometimes need to use GetOkExists for booleans
643+
if val, ok := webCommitSignoffRequired.(bool); ok {
644+
repository.WebCommitSignoffRequired = github.Ptr(val)
645+
}
646+
}
647+
}
648+
641649
return repository
642650
}
643651

@@ -947,20 +955,6 @@ func resourceGithubRepositoryUpdate(ctx context.Context, d *schema.ResourceData,
947955
owner := meta.(*Owner).name
948956
ctx = context.WithValue(ctx, ctxId, d.Id())
949957

950-
// When the organization has "Require sign off on web-based commits" enabled,
951-
// the API doesn't allow you to send `web_commit_signoff_required` in order to
952-
// update the repository with this field or it will throw a 422 error.
953-
// As a workaround, we check if the organization requires it, and if so,
954-
// we remove the field from the request.
955-
if d.HasChange("web_commit_signoff_required") && meta.(*Owner).IsOrganization {
956-
organization, _, err := client.Organizations.Get(ctx, owner)
957-
if err == nil {
958-
if organization != nil && organization.GetWebCommitSignoffRequired() {
959-
repoReq.WebCommitSignoffRequired = nil
960-
}
961-
}
962-
}
963-
964958
repo, _, err := client.Repositories.Edit(ctx, owner, repoName, repoReq)
965959
if err != nil {
966960
return diag.FromErr(err)
@@ -1043,8 +1037,6 @@ func resourceGithubRepositoryDelete(ctx context.Context, d *schema.ResourceData,
10431037
return diag.FromErr(err)
10441038
}
10451039
repoReq := resourceGithubRepositoryObject(d)
1046-
// Always remove `web_commit_signoff_required` when archiving, to avoid 422 error
1047-
repoReq.WebCommitSignoffRequired = nil
10481040
log.Printf("[DEBUG] Archiving repository on delete: %s/%s", owner, repoName)
10491041
_, _, err := client.Repositories.Edit(ctx, owner, repoName, repoReq)
10501042
return diag.FromErr(err)

‎github/resource_github_repository_test.go‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1260,6 +1260,111 @@ resource "github_repository" "test" {
12601260
})
12611261
})
12621262

1263+
t.Run("check_web_commit_signoff_required_enabled", func(t *testing.T) {
1264+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
1265+
testRepoName := fmt.Sprintf("%scommit-signoff-%s", testResourcePrefix, randomID)
1266+
config := `
1267+
resource "github_repository" "test" {
1268+
name = "%s"
1269+
auto_init = true
1270+
web_commit_signoff_required = %s
1271+
}
1272+
`
1273+
1274+
resource.Test(t, resource.TestCase{
1275+
PreCheck: func() { skipUnauthenticated(t) },
1276+
ProviderFactories: providerFactories,
1277+
Steps: []resource.TestStep{
1278+
{
1279+
Config: fmt.Sprintf(config, testRepoName, "true"),
1280+
Check: resource.ComposeTestCheckFunc(
1281+
resource.TestCheckResourceAttr("github_repository.test", "web_commit_signoff_required", "true"),
1282+
),
1283+
},
1284+
},
1285+
})
1286+
})
1287+
1288+
t.Run("check_web_commit_signoff_required_disabled", func(t *testing.T) {
1289+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
1290+
testRepoName := fmt.Sprintf("%scommit-signoff-%s", testResourcePrefix, randomID)
1291+
config := `
1292+
resource "github_repository" "test" {
1293+
name = "%s"
1294+
auto_init = true
1295+
web_commit_signoff_required = %s
1296+
}
1297+
`
1298+
1299+
resource.Test(t, resource.TestCase{
1300+
PreCheck: func() { skipUnauthenticated(t) },
1301+
ProviderFactories: providerFactories,
1302+
Steps: []resource.TestStep{
1303+
{
1304+
Config: fmt.Sprintf(config, testRepoName, "false"),
1305+
Check: resource.ComposeTestCheckFunc(
1306+
resource.TestCheckResourceAttr("github_repository.test", "web_commit_signoff_required", "false"),
1307+
),
1308+
},
1309+
},
1310+
})
1311+
})
1312+
1313+
t.Run("check_web_commit_signoff_required_not_set", func(t *testing.T) {
1314+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
1315+
testRepoName := fmt.Sprintf("%scommit-signoff-%s", testResourcePrefix, randomID)
1316+
config := `
1317+
resource "github_repository" "test" {
1318+
name = "%s"
1319+
auto_init = true
1320+
}
1321+
`
1322+
1323+
resource.Test(t, resource.TestCase{
1324+
PreCheck: func() { skipUnauthenticated(t) },
1325+
ProviderFactories: providerFactories,
1326+
Steps: []resource.TestStep{
1327+
{
1328+
Config: fmt.Sprintf(config, testRepoName),
1329+
Check: resource.ComposeTestCheckFunc(
1330+
resource.TestCheckResourceAttr("github_repository.test", "web_commit_signoff_required", "false"),
1331+
),
1332+
},
1333+
},
1334+
})
1335+
})
1336+
1337+
t.Run("check_web_commit_signoff_required_organization_enabled_but_not_set", func(t *testing.T) {
1338+
t.Skip("This test should be run manually after confirming that the test organization has 'Require contributors to sign off on web-based commits' enabled under Organizations -> Settings -> Repository -> Repository defaults.")
1339+
1340+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
1341+
testRepoName := fmt.Sprintf("%scommit-signoff-%s", testResourcePrefix, randomID)
1342+
1343+
config := `
1344+
resource "github_repository" "test" {
1345+
name = "%s"
1346+
description = "%s"
1347+
visibility = "private"
1348+
}
1349+
`
1350+
1351+
resource.Test(t, resource.TestCase{
1352+
PreCheck: func() { skipUnauthenticated(t) },
1353+
ProviderFactories: providerFactories,
1354+
Steps: []resource.TestStep{
1355+
{
1356+
Config: fmt.Sprintf(config, testRepoName, "foo"),
1357+
Check: resource.ComposeTestCheckFunc(
1358+
resource.TestCheckResourceAttr("github_repository.test", "web_commit_signoff_required", "true"),
1359+
),
1360+
},
1361+
{
1362+
Config: fmt.Sprintf(config, testRepoName, "bar"),
1363+
},
1364+
},
1365+
})
1366+
})
1367+
12631368
t.Run("check_allow_forking_not_set", func(t *testing.T) {
12641369
t.Skip("This test should be run manually after confirming that the test organization has been correctly configured to disable setting forking at the repo level.")
12651370

‎website/docs/r/repository.html.markdown‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ The following arguments are supported:
112112

113113
* `delete_branch_on_merge` - (Optional) Automatically delete head branch after a pull request is merged. Defaults to `false`.
114114

115-
* `web_commit_signoff_required` - (Optional) Require contributors to sign off on web-based commits. See more [here](https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/managing-the-commit-signoff-policy-for-your-repository). Defaults to `false`.
115+
* `web_commit_signoff_required` - (Optional) Require contributors to sign off on web-based commits. See more [here](https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/managing-the-commit-signoff-policy-for-your-repository).
116116

117117
* `has_downloads` - (**DEPRECATED**) (Optional) Set to `true` to enable the (deprecated) downloads features on the repository. This attribute is no longer in use, but it hasn't been removed yet. It will be removed in a future version. See [this discussion](https://github.com/orgs/community/discussions/102145#discussioncomment-8351756).
118118

0 commit comments

Comments
 (0)