-
Notifications
You must be signed in to change notification settings - Fork 763
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
Explore setting vulnerability alerts correctly #768
Explore setting vulnerability alerts correctly #768
Conversation
Update: all repo tests pass for me with the following command off of this branch: |
Terraform file used for testing: terraform {
required_providers {
github = {
source = "integrations/github"
version = "4.8.0"
}
}
required_version = ">= 0.14"
}
resource "github_repository" "test-754" {
visibility = "private"
name = "test-754-1"
delete_branch_on_merge = true
has_issues = false
has_projects = false
has_wiki = false
vulnerability_alerts = true
} Test cases performed:
|
I've removed the vulnerability alert handling from the initial @jcudit I think this is ready for review now. Would you like to pair with me sometime this week to test this on an enterprise instance? |
Note also that it appears that the only reason the |
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.
Makes sense to me. Thanks for taking the time to pull the historical context.
Any movement on this? Looks like it may fix #716 |
@@ -99,7 +99,7 @@ initial repository creation and create the target branch inside of the repositor | |||
|
|||
* `template` - (Optional) Use a template repository to create this resource. See [Template Repositories](#template-repositories) below for details. | |||
|
|||
* `vulnerability_alerts` (Optional) - 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.) See [GitHub Documentation](https://help.github.com/en/github/managing-security-vulnerabilities/about-security-alerts-for-vulnerable-dependencies) for details. | |||
* `vulnerability_alerts` (Optional) - 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.) See [GitHub Documentation](https://help.github.com/en/github/managing-security-vulnerabilities/about-security-alerts-for-vulnerable-dependencies) for details. Note that vulnerability alerts have not been successfully tested on any GitHub Enterprise instance and may be unavailable in those settings. |
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.
We have been unable to set vulnerability alerts successfully in multiple GitHub Enterprise instances. I've updated the docs to say so with the following wording:
Note that vulnerability alerts have not been successfully tested on any GitHub Enterprise instance and may be unavailable in those settings.
I'm happy to change the wording as desired!
* Initial commit of setting vulnerability_alerts * Remove vulnerability alerts handling from create and keep on update * Add note for unsuccessful enterprise vulnerability alert setting
Removing the
!d.isNewResource()
check seems to fix vulnerability alerts setting for me (as mentioned in issue 754). I'm making this PR in hopes that it doesn't mess up the existing tests...I'm wondering why that behavior was there in the first place. Perhaps it's something to do with the create/immediate update flow we use.I'm a little hesitant to remove that check due to Chesterton's fence, but perhaps someone else knows more than I do about it, and the testing coverage should help us as well.
TODO: