-
Notifications
You must be signed in to change notification settings - Fork 783
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
base: main
Are you sure you want to change the base?
Conversation
9d729e8
to
611ed13
Compare
Signed-off-by: Steve Hipwell <[email protected]>
611ed13
to
1361601
Compare
@@ -320,6 +320,7 @@ func resourceGithubRepository() *schema.Resource { | |||
"vulnerability_alerts": { | |||
Type: schema.TypeBool, | |||
Optional: true, | |||
Computed: true, |
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.
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.
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.
It is also required in combination with optional
if an optional value will be persisted even if it's not been explicitly set.
@@ -619,6 +619,11 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta interface{}) er | |||
} | |||
} | |||
|
|||
err := updateVulnerabilityAlerts(d, client, ctx, owner, repoName) |
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.
This seems wrong, the return resourceGithubRepositoryRead(d, meta)
line below can end up calling updateVulnerabilityAlerts
again. Is this change actually needed?
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.
This part of the TF provider is a bit of a mess, AFAIK 3 months later this is required.
|
||
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) { |
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.
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?
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.
@Lekensteyn this is a pattern used throughout the provider, the GetOk()
function is intended to only respond if the value has been set.
Resolves #2489
Resolves #2495
Before the change?
github_repository
resource churned onvulnerability_alerts
github_repository
resource churned on pages if build type was workflowAfter the change?
github_repository
resourcePull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!