Skip to content
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

fix: Fixed repository resource churn #2501

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 31 additions & 14 deletions github/resource_github_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ func resourceGithubRepository() *schema.Resource {
"vulnerability_alerts": {
Type: schema.TypeBool,
Optional: true,
Computed: true,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the documentation, "Computed is often used to represent values that are not user configurable or can not be known at time of terraform plan or apply, such as date of creation or a service specific UUID."

I suppose that Computed is appropriate here since it could be enabled by default at the org level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also required in combination with optional if an optional value will be persisted even if it's not been explicitly set.

Description: "Set to 'true' to enable security alerts for vulnerable dependencies. Enabling requires alerts to be enabled on the owner level. (Note for importing: GitHub enables the alerts on public repos but disables them on private repos by default). Note that vulnerability alerts have not been successfully tested on any GitHub Enterprise instance and may be unavailable in those settings.",
},
"ignore_vulnerability_alerts_during_read": {
Expand Down Expand Up @@ -412,7 +413,6 @@ func resourceGithubRepository() *schema.Resource {
}

func calculateVisibility(d *schema.ResourceData) string {

if value, ok := d.GetOk("visibility"); ok {
return value.(string)
}
Expand Down Expand Up @@ -619,6 +619,11 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta interface{}) er
}
}

err := updateVulnerabilityAlerts(d, client, ctx, owner, repoName)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong, the return resourceGithubRepositoryRead(d, meta) line below can end up calling updateVulnerabilityAlerts again. Is this change actually needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the TF provider is a bit of a mess, AFAIK 3 months later this is required.

if err != nil {
return err
}

return resourceGithubRepositoryUpdate(d, meta)
}

Expand Down Expand Up @@ -817,12 +822,7 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta interface{}) er
}

if d.HasChange("vulnerability_alerts") {
updateVulnerabilityAlerts := client.Repositories.DisableVulnerabilityAlerts
if vulnerabilityAlerts, ok := d.GetOk("vulnerability_alerts"); ok && vulnerabilityAlerts.(bool) {
updateVulnerabilityAlerts = client.Repositories.EnableVulnerabilityAlerts
}

_, err = updateVulnerabilityAlerts(ctx, owner, repoName)
err = updateVulnerabilityAlerts(d, client, ctx, owner, repoName)
if err != nil {
return err
}
Expand Down Expand Up @@ -957,13 +957,19 @@ func flattenPages(pages *github.Pages) []interface{} {
return []interface{}{}
}

sourceMap := make(map[string]interface{})
sourceMap["branch"] = pages.GetSource().GetBranch()
sourceMap["path"] = pages.GetSource().GetPath()

pagesMap := make(map[string]interface{})
pagesMap["source"] = []interface{}{sourceMap}
pagesMap["build_type"] = pages.GetBuildType()
buildType := pages.GetBuildType()
pagesMap["build_type"] = buildType

if buildType == "legacy" {
sourceMap := make(map[string]interface{})
sourceMap["branch"] = pages.GetSource().GetBranch()
sourceMap["path"] = pages.GetSource().GetPath()
pagesMap["source"] = []interface{}{sourceMap}
} else {
pagesMap["source"] = nil
}

pagesMap["url"] = pages.GetURL()
pagesMap["status"] = pages.GetStatus()
pagesMap["cname"] = pages.GetCNAME()
Expand Down Expand Up @@ -1038,7 +1044,8 @@ func flattenSecurityAndAnalysis(securityAndAnalysis *github.SecurityAndAnalysis)
// resourceGithubParseFullName will return "myorg", "myrepo", true when full_name is "myorg/myrepo".
func resourceGithubParseFullName(resourceDataLike interface {
GetOk(string) (interface{}, bool)
}) (string, string, bool) {
},
) (string, string, bool) {
x, ok := resourceDataLike.GetOk("full_name")
if !ok {
return "", "", false
Expand All @@ -1062,3 +1069,13 @@ func customDiffFunction(_ context.Context, diff *schema.ResourceDiff, v interfac
}
return nil
}

func updateVulnerabilityAlerts(d *schema.ResourceData, client *github.Client, ctx context.Context, owner, repoName string) error {
updateVulnerabilityAlerts := client.Repositories.DisableVulnerabilityAlerts
if vulnerabilityAlerts, ok := d.GetOk("vulnerability_alerts"); ok && vulnerabilityAlerts.(bool) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not new behavior, but this treats the absense of vulnerability_alerts = "true" as an alias for vulnerability_alerts = "false".

That is not true when a global org-level policy enables vulnerability alerts by default. Is this desirable behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lekensteyn this is a pattern used throughout the provider, the GetOk() function is intended to only respond if the value has been set.

updateVulnerabilityAlerts = client.Repositories.EnableVulnerabilityAlerts
}

_, err := updateVulnerabilityAlerts(ctx, owner, repoName)
return err
}
Loading