Bugfix: Make security_and_analysis settings optional

[jtgrohn]

Resolves #1419
Resolves #1466

---

Behavior

Before the change?

The security_and_analysis settings were all required.

  security_and_analysis {
    advanced_security {
      status = "enabled"
    }
    secret_scanning {
      status = "enabled"
    }
    secret_scanning_push_protection {
      status = "enabled"
    }
  }

This resulted in failures for public repos where it is not possible to change the Advanced Settings (see #1419)

Error: PATCH https://api.github.com/repos/ministryofjustice/modernisation-platform: 422 Advanced security is always available for public repos []

Additional testing revealed that the advanced_security cannot always be set for private/internal repositories and that secret_scanning and secret_scanning_push_protection settings also can only be set in certain scenarios (e.g. Advanced Security must be available (purchased) for organizations/private repos)

Error: PATCH https://api.github.com/repos/jtgrohn-terraform-provider-github-test/tf-acc-00twu: 422 Advanced security has not been purchased []

Error: PATCH https://api.github.com/repos/jtgrohn-terraform-provider-github-test/tf-acc-8cq73: 422 Secret scanning is not available for this repository []

Additionally, before this change updating the provider attempted to remove the security_and_analysis settings if it was not configured in the terraform config (see #1466)

Terraform will perform the following actions:

  # github_repository.repo will be updated in-place
  ~ resource "github_repository" "repo" {
        id                          = "security_and_analysis_test"
        name                        = "security_and_analysis_test"
      - vulnerability_alerts        = true -> null
        # (28 unchanged attributes hidden)

      - security_and_analysis {
          - advanced_security {
              - status = "enabled" -> null
            }

          - secret_scanning {
              - status = "enabled" -> null
            }

          - secret_scanning_push_protection {
              - status = "enabled" -> null
            }
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

After the change?

After the change, the security_and_analysis settings are all optional. In addition, they have been changed to computed properties and they are set with the initial repo creation request, not edited after it has been created. If the security_and_analysis settings are not supplied in the terraform config they are not sent along to GitHub (whereas previously they were all set to disabled).

The intent is to let the GitHub API tell us when we can and can't do stuff, instead of trying to re-encode that within the provider. So this change would allow someone to create a terraform config that will pass validate/plan, but will fail to apply (which is already the case before this change, but now there are a few additional ways to do so). I think this is okay though since the fix is simply to set the security_and_analysis sub properties appropriately based on the error messages the GitHub API returns. Before this change, that is not always possible - the module is completely broken for public repos.

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Added the appropriate label for the given change

Does this introduce a breaking change?

• Yes (Please add the Type: Breaking change label)
• No

[kfcampbell]

I am curious about the reason for requiring interfaces rather than a concrete type, but this makes great updates to our repository resource and its tests, which are passing after this PR! Thank you for the contribution.

[kfcampbell]

What's the reason this can't be a concrete type here?

[jtgrohn]

A concrete type could be introduced so that there is one, but the flattenSecurityAndAnalysis function return type previously was []interface{}, so I kept it the same when I was refactoring it / making the properties optional.

I also removed the expandSecurityAndAnalysis function. The diff is lining the changes with flattenSecurityAndAnalysis up with the expandSecurityAndAnalysis removal.

[kfcampbell]

Confirming that executing the examples succeeds as planned.