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

Explore setting vulnerability alerts correctly #768

Merged

Conversation

kfcampbell
Copy link
Member

@kfcampbell kfcampbell commented Apr 22, 2021

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:

  • validate tests still pass
  • provide validation instructions for manual testing

@kfcampbell
Copy link
Member Author

Update: all repo tests pass for me with the following command off of this branch: TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubRepositories.

@kfcampbell
Copy link
Member Author

kfcampbell commented Apr 22, 2021

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:

  • Starting with a private repo, vulnerability alerts off
    • Correctly started with vulnerability alerts off
    • Turning vulnerability alerts on works correctly
    • Rerunning the same terraform file causes no changes
  • Starting with a private repo, vulnerability alerts on
    • Correctly started with vulnerability alerts on
    • Turning vulnerability alerts off works correctly
    • Rerunning the same terraform file causes no changes

@kfcampbell
Copy link
Member Author

I've removed the vulnerability alert handling from the initial Create call and moved it entirely to the Update call that immediately follows. I've replicated all of the tests from this comment as well as the integration tests, and everything looks like it's passing to me.

@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?

@kfcampbell kfcampbell marked this pull request as ready for review April 25, 2021 17:59
@kfcampbell
Copy link
Member Author

Note also that it appears that the only reason the !d.isNewResource() call was added was an attempt to save an API call. See the historical context here.

Copy link
Contributor

@majormoses majormoses left a 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.

@joshmyers
Copy link

Any movement on this? Looks like it may fix #716

@jcudit jcudit added this to the v4.13.0 milestone Jul 22, 2021
@@ -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.
Copy link
Member Author

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!

@jcudit jcudit merged commit 62577af into integrations:main Jul 26, 2021
@kfcampbell kfcampbell deleted the vulnerability-alerts-investigation branch July 26, 2021 14:30
kfcampbell added a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
* Initial commit of setting vulnerability_alerts

* Remove vulnerability alerts handling from create and keep on update

* Add note for unsuccessful enterprise vulnerability alert setting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants