[Fix]: Advanced security settings for public repos.

[kuhlman-labs]

Resolves #1419

---

Behavior

Before the change?

• Creating a public repo resource would error out if advanced security settings were set with the following message:

"Advanced security is always available for public repos"

After the change?

• This change adds logic so that if a repos visibility is set to public it will not include the advanced security field in the payload but still allows setting the other fields.

Other information

---

Additional info

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?

Please see our docs on breaking changes to help!

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

If Yes, what's the impact:

• N/A

Pull request type

Please add the corresponding label for change this PR introduces:

• Bugfix: Type: Bug
• Feature/model/API additions: Type: Feature
• Updates to docs or samples: Type: Documentation
• Dependencies/code cleanup: Type: Maintenance

---

[sethvargo]

I'm not sure this is the correct approach. We should only apply the advanced_security settings if they were explicitly set in the HCL. Right now, setting the value in HCL has no effect (with no warning or anything).

I recommend using GetOk instead of Get on "security_and_analysis" and only adding the value if set.

[kuhlman-labs]

I considered this approach but I decided to do it this way for a couple of reasons:

• The only time where the advanced_security field does not need to be set us for public repositories because it is enabled by default.
• In order to use the GetOk approach I would need to set that field as optional which may cause confusion for users in other scenarios.
• I think there is some value in seeing that the advanced_security field is set in the HCL even for public repos so practitioners can see at a glance that the repo is set to use advanced security.

[sethvargo]

The only time where the advanced_security field does not need to be set us for public repositories because it is enabled by default.

That is true as of today. It could change in the future and then suddenly things break in an unexpected way.

In order to use the GetOk approach I would need to set that field as optional which may cause confusion for users in other scenarios.

I'm not sure why you'd need to set it as optional to use GetOk? I also think that's correct because it is optional. It's not required for public or private repositories:

I think there is some value in seeing that the advanced_security field is set in the HCL even for public repos so practitioners can see at a glance that the repo is set to use advanced security.

This is all rooted in the assumption that "public == advanced security analysis", and I think that's a fundamentally flawed assumption.

[kuhlman-labs]

I think we're talking about two different things here.

I'm not sure why you'd need to set it as optional to use GetOk? I also think that's correct because it is optional. It's not required for public or private repositories:

The block for security_and_analysis is currently optional, if it is not set in the HCL then we don't send the call to configure those settings. However, if you do set the security_and_analysis block in your HCL all of the subfields are required advanced_security, secret_scanning, and secret_scanning_push_protection. The issue when setting the security_and_analysis block for public repos is it would error out because by default advanced_security is enabled and sending that field in the payload would give the error message "Advanced security is always available for public repos". However you can still configure the secret_scanning, and secret_scanning_push_protection features for a public repo. To do a GetOk check I would need to change the advanced_security field to optional which I think would cause some confusion in other scenarios as that field needs to be set to enabled to be able to enable the secret_scanning, and secret_scanning_push_protection fields.

[sethvargo]

I understand what you're saying, but I don't think this fixes the bug. See

terraform-provider-github/github/resource_github_repository.go

Lines 635 to 644 in 48619d4

	} else { // disable security and analysis
	_, _, err := client.Repositories.Edit(ctx, owner, repoName, &github.Repository{
	SecurityAndAnalysis: &github.SecurityAndAnalysis{
	AdvancedSecurity: &github.AdvancedSecurity{
	Status: github.String("disabled")},
	SecretScanning: &github.SecretScanning{
	Status: github.String("disabled")},
	SecretScanningPushProtection: &github.SecretScanningPushProtection{
	Status: github.String("disabled")}},
	})

, as an example. If the github_repository resource was already under management and the user deletes the entire security_and_analysis section, then Terraform will attempt to update the repo to disable Advanced Security (L638). But that's not valid for public repos.

My point is that you're headed down a path where you must explicitly check for repo visibility for any calls that modify security_and_analysis. Not only is this very error-prone, but I think it's the wrong design here.

[kuhlman-labs]

I've pushed a change that will add some logic to check the repo visibility when removing the security_and_analysis block and send the appropriate payload. Thanks for catching that!

Why do you see the check for visibility as error prone? What approach would you recommend?

[sethvargo]

Why do you see the check for visibility as error prone?

This comment thread is literally an example of why it's error prone 😄. I think a better pattern is to centralize the logic for compiling the SecurityAndAnalysis block into a single function that takes the current state, previous state, and repo visibility into consideration when building the structure. Then use that function everywhere and throw a ton of unit tests at it to make sure it behaves as expected under all possible conditions.

[usmonster]

I guess this is superseded by #1489 and can be closed?

[kfcampbell]

@usmonster I believe that is correct, though I'll leave it to @kuhlman-labs to close the PR.