[BUG] Ensure repository creation works if vulnerability-alerts isn't modifiable on repo level #3024
Conversation
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
In the case of `vulnerability_alerts = false` and the message indicates control from parent Org/Enterprise Signed-off-by: Timo Sand <timo.sand@f-secure.com>
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
| _, err := updateVulnerabilityAlerts(ctx, owner, repoName) | ||
| resp, err := updateVulnerabilityAlertsSDK(ctx, owner, repoName) | ||
| if err != nil { | ||
| if resp.StatusCode == http.StatusUnprocessableEntity && strings.Contains(err.Error(), "An enforced security configuration prevented modifying") && !ok { |
There was a problem hiding this comment.
This would be a brittle fix, the documented REST API is hard enough to work with so using an error message to control logic isn't something I'd be comfortable doing.
I think the current behaviour is defective given the subsequent GitHub changes. Based on my understanding of d.GetOK() a change like the one I suggested above would only impact new public repos where there is no organization controlled default.
There was a problem hiding this comment.
I did try using d.GetOk() and skipping Delete/Enable with a non-paid Org and private repo. That left the state in a weird state, as vulnerability_alerts was enabled by default and TF had trouble consolidating configuration vs remote state.
I agree that using the error message is problematic, but I can't see any other way to do this with our current limitations.
There are many other cases where a 422 status code shouldn't be ignored, so we can't base it on that either
There was a problem hiding this comment.
Actually there isn't a good solution for this, even with my pattern the API will be hit every time there is an unrelated repo change. I guess we could remove computed and leave it null if not set, but that is a bigger change. Based on that I think we'll have to go with the error checking logic, but I think we should do the following as part of this PR.
- Add warning comment in the code that it's a temporary workaround
- Add issue to track adding a new resource for vulnerability alerts, adding a new
ignore_vulnerability_alertsfield and deprecatingvulnerability_alerts&ignore_vulnerability_alerts_during_readforv6.10.0 - Add issue to track removal of
ignore_vulnerability_alerts,vulnerability_alerts&ignore_vulnerability_alerts_during_readforv7.0.0
There was a problem hiding this comment.
@stevehipwell Done!
I've added #3038 and #3039 could you assign them to correct milestones?
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
2 participants
Resolves #2988
Before the change?
resource_github_repository.gowas using Context-unaware functionsupdateVulnerabilityAlertswould cause terraform to failAfter the change?
resource_github_repository.gois using Context-aware functionsupdateVulnerabilityAlertsto ensure repo creation works if vulnerability alerts are not modifiablePull request checklist
Schema migrations have been created if needed (example)Docs have been reviewed and added / updated if needed (for bug fixes / features)Does this introduce a breaking change?
Please see our docs on breaking changes to help!