-
Notifications
You must be signed in to change notification settings - Fork 754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add vulnerability_alerts attribute for repositories #444
Changes from 1 commit
7dd7b9d
244f079
11651f6
c4916dd
059fa1e
cb87733
f71e18f
40b17dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,6 +118,11 @@ func resourceGithubRepository() *schema.Resource { | |
ValidateFunc: validation.StringMatch(regexp.MustCompile(`^[a-z0-9][a-z0-9-]*$`), "must include only lowercase alphanumeric characters or hyphens and cannot start with a hyphen"), | ||
}, | ||
}, | ||
"vulnerability_alerts": { | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
Default: false, | ||
}, | ||
|
||
"full_name": { | ||
Type: schema.TypeString, | ||
|
@@ -262,6 +267,26 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta interface{}) er | |
} | ||
} | ||
|
||
var alerts, private bool | ||
if a, ok := d.GetOk("vulnerability_alerts"); ok { | ||
alerts = a.(bool) | ||
} | ||
if p, ok := d.GetOk("private"); ok { | ||
private = p.(bool) | ||
} | ||
var createVulnerabilityAlerts func(context.Context, string, string) (*github.Response, error) | ||
if private && alerts { | ||
createVulnerabilityAlerts = client.Repositories.EnableVulnerabilityAlerts | ||
} else if !private && !alerts { | ||
createVulnerabilityAlerts = client.Repositories.DisableVulnerabilityAlerts | ||
} | ||
if createVulnerabilityAlerts != nil { | ||
_, err = createVulnerabilityAlerts(ctx, orgName, repoName) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return resourceGithubRepositoryUpdate(d, meta) | ||
} | ||
|
||
|
@@ -334,6 +359,12 @@ func resourceGithubRepositoryRead(d *schema.ResourceData, meta interface{}) erro | |
d.Set("template", []interface{}{}) | ||
} | ||
|
||
vulnerabilityAlerts, _, err := client.Repositories.GetVulnerabilityAlerts(ctx, orgName, repoName) | ||
if err != nil { | ||
return fmt.Errorf("Error reading repository vulnerability alerts: %v", err) | ||
} | ||
d.Set("vulnerability_alerts", vulnerabilityAlerts) | ||
|
||
return nil | ||
} | ||
|
||
|
@@ -374,6 +405,18 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta interface{}) er | |
} | ||
} | ||
|
||
if d.HasChange("vulnerability_alerts") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my only suggestion here could be to save an API call after creation of a resource by adding the |
||
updateVulnerabilityAlerts := client.Repositories.DisableVulnerabilityAlerts | ||
if vulnerabilityAlerts, ok := d.GetOk("vulnerability_alerts"); ok && vulnerabilityAlerts.(bool) { | ||
updateVulnerabilityAlerts = client.Repositories.EnableVulnerabilityAlerts | ||
} | ||
|
||
_, err = updateVulnerabilityAlerts(ctx, orgName, repoName) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return resourceGithubRepositoryRead(d, meta) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,6 +83,7 @@ func TestAccGithubRepository_basic(t *testing.T) { | |
DefaultBranch: "master", | ||
Archived: false, | ||
}), | ||
testAccCheckGithubVulnerabilityAlerts(rn, false), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in calls to |
||
), | ||
}, | ||
{ | ||
|
@@ -101,6 +102,7 @@ func TestAccGithubRepository_basic(t *testing.T) { | |
HasProjects: false, | ||
Archived: false, | ||
}), | ||
testAccCheckGithubVulnerabilityAlerts(rn, false), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similar comment as above |
||
), | ||
}, | ||
{ | ||
|
@@ -523,6 +525,78 @@ func TestAccGithubRepository_createFromTemplate(t *testing.T) { | |
}) | ||
} | ||
|
||
func TestAccGithubRepository_vulnerabilityAlerts(t *testing.T) { | ||
var repo github.Repository | ||
|
||
rn := "github_repository.foo" | ||
randString := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum) | ||
name := fmt.Sprintf("tf-acc-test-%s", randString) | ||
description := fmt.Sprintf("Terraform acceptance tests %s", randString) | ||
|
||
resource.ParallelTest(t, resource.TestCase{ | ||
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckGithubRepositoryDestroy, | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccGithubRepositoryVulnerabilityAlertsConfig(randString), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckGithubRepositoryExists(rn, &repo), | ||
testAccCheckGithubRepositoryAttributes(&repo, &testAccGithubRepositoryExpectedAttributes{ | ||
Name: name, | ||
Description: description, | ||
Homepage: "http://example.com/", | ||
HasIssues: true, | ||
HasWiki: true, | ||
IsTemplate: false, | ||
AllowMergeCommit: true, | ||
AllowSquashMerge: false, | ||
AllowRebaseMerge: false, | ||
DeleteBranchOnMerge: false, | ||
HasDownloads: true, | ||
HasProjects: false, | ||
DefaultBranch: "master", | ||
Archived: false, | ||
}), | ||
testAccCheckGithubVulnerabilityAlerts(rn, true), | ||
), | ||
}, | ||
{ | ||
Config: testAccGithubRepositoryVulnerabilityAlertsUpdateConfig(randString), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckGithubRepositoryExists(rn, &repo), | ||
|
||
testAccCheckGithubRepositoryAttributes(&repo, &testAccGithubRepositoryExpectedAttributes{ | ||
Name: name, | ||
Description: description, | ||
Homepage: "http://example.com/", | ||
HasIssues: true, | ||
HasWiki: true, | ||
IsTemplate: false, | ||
AllowMergeCommit: true, | ||
AllowSquashMerge: false, | ||
AllowRebaseMerge: false, | ||
DeleteBranchOnMerge: false, | ||
HasDownloads: true, | ||
HasProjects: false, | ||
DefaultBranch: "master", | ||
Archived: false, | ||
}), | ||
testAccCheckGithubVulnerabilityAlerts(rn, false), | ||
), | ||
}, | ||
{ | ||
ResourceName: rn, | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
ImportStateVerifyIgnore: []string{ | ||
"auto_init", | ||
}, | ||
}, | ||
}, | ||
}) | ||
} | ||
|
||
func testAccCheckGithubRepositoryExists(n string, repo *github.Repository) resource.TestCheckFunc { | ||
return func(s *terraform.State) error { | ||
rs, ok := s.RootModule().Resources[n] | ||
|
@@ -557,6 +631,33 @@ func testAccCheckGithubRepositoryTemplateRepoAttribute(n string, repo *github.Re | |
} | ||
} | ||
|
||
func testAccCheckGithubVulnerabilityAlerts(n string, expected bool) resource.TestCheckFunc { | ||
return func(s *terraform.State) error { | ||
rs, ok := s.RootModule().Resources[n] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it may be redundant to check w/in the state here as a call to this function in the acceptance tests is preceded by |
||
if !ok { | ||
return fmt.Errorf("Not Found: %s", n) | ||
} | ||
|
||
repoName := rs.Primary.ID | ||
if repoName == "" { | ||
return fmt.Errorf("No repository name is set") | ||
} | ||
|
||
org := testAccProvider.Meta().(*Organization) | ||
conn := org.v3client | ||
actual, _, err := conn.Repositories.GetVulnerabilityAlerts(context.TODO(), org.name, repoName) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if expected != actual { | ||
return fmt.Errorf("Unexpected vulnerability alerts, got: %t", actual) | ||
} | ||
|
||
return nil | ||
} | ||
} | ||
|
||
type testAccGithubRepositoryExpectedAttributes struct { | ||
Name string | ||
Description string | ||
|
@@ -965,3 +1066,53 @@ resource "github_branch_protection" "repo_name_master" { | |
} | ||
`, randString) | ||
} | ||
|
||
func testAccGithubRepositoryVulnerabilityAlertsConfig(randString string) string { | ||
return fmt.Sprintf(` | ||
resource "github_repository" "foo" { | ||
name = "tf-acc-test-%s" | ||
description = "Terraform acceptance tests %s" | ||
homepage_url = "http://example.com/" | ||
|
||
# So that acceptance tests can be run in a github organization | ||
# with no billing | ||
private = false | ||
|
||
has_issues = true | ||
has_wiki = true | ||
is_template = false | ||
allow_merge_commit = true | ||
allow_squash_merge = false | ||
allow_rebase_merge = false | ||
has_downloads = true | ||
auto_init = false | ||
|
||
vulnerability_alerts = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: can be aligned by using |
||
} | ||
`, randString, randString) | ||
} | ||
|
||
func testAccGithubRepositoryVulnerabilityAlertsUpdateConfig(randString string) string { | ||
return fmt.Sprintf(` | ||
resource "github_repository" "foo" { | ||
name = "tf-acc-test-%s" | ||
description = "Terraform acceptance tests %s" | ||
homepage_url = "http://example.com/" | ||
|
||
# So that acceptance tests can be run in a github organization | ||
# with no billing | ||
private = false | ||
|
||
has_issues = true | ||
has_wiki = true | ||
is_template = false | ||
allow_merge_commit = true | ||
allow_squash_merge = false | ||
allow_rebase_merge = false | ||
has_downloads = true | ||
auto_init = false | ||
|
||
vulnerability_alerts = false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same nit as above |
||
} | ||
`, randString, randString) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my concern here is, as stated, this default value is only applicable to
private
repos as initially defined by the API, and a user may unintentionally disable alerts for apublic
repo by omitting this attribute in a config and making it through theapply
step..could we consider leaving this w/o a default or, given the feedback in the original issue, is the general user experience better with having this value in place?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 my vote goes to removing the default.