Explore setting vulnerability alerts correctly

[kfcampbell]

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]

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]

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]

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]

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.

[majormoses]

Makes sense to me. Thanks for taking the time to pull the historical context.

[joshmyers]

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

[kfcampbell]

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!