Run credentials validation only once #2949
Conversation
Does the PR have any schema changes?Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
|
Thank you this looks good. This is an incredibly high-visibility issue, and we do not want to regress on this ever again, so in light of this please add a ProgramTest level coverage that asserts that the expected warning falls out of the un-configured provider. TYVM. |
provider/resources.go
Outdated
| @@ -626,6 +615,30 @@ func preConfigureCallback(vars resource.PropertyMap, c shim.ResourceConfig) erro | |||
| return nil | |||
| } | |||
|
|
|||
| // We should only run the validation once to avoid duplicating the reported errors. | |||
| var credentialsValidationCounter atomic.Int32 | |||
There was a problem hiding this comment.
Nit: We have atomic.Bool with if !credentialsValidated.CompareAndSwap(false, true) { return nil }
We also have sync.OnceFunc that exists to do exactly what you have implemented in atomics.
Neither of these options are vulnerable to overflow (though I doubt that will be a problem).
There was a problem hiding this comment.
sync.Once is nice actually, I forgot that one.
There was a problem hiding this comment.
Great suggestion, thank you! I think OnceFunc would be a bit tricky to use here but sync.Once worked well and made it clearer.
| return nil | ||
| } | ||
|
|
||
| func validateCredentials(vars resource.PropertyMap, c shim.ResourceConfig) error { |
There was a problem hiding this comment.
I appreciate the separation here.
provider/resources.go
Outdated
| return nil | ||
| } | ||
|
|
||
| var err error = nil |
There was a problem hiding this comment.
The zero value is implicit.
| var err error = nil | |
| var err error |
|
Looks like tests failed due to the GH outage. I'm re-running now |
similar to pulumi/pulumi-aws#2949 Run preconfigure check only once. This removes duplication of config warnings. Before: ``` Diagnostics: pulumi:pulumi:Stack (gcp_vm-dev): warning: unable to detect a global setting for GCP Project; Pulumi will rely on per-resource settings for this operation. Set the GCP Project by using: `pulumi config set gcp:project <project>` warning: unable to detect a global setting for GCP Project; Pulumi will rely on per-resource settings for this operation. Set the GCP Project by using: `pulumi config set gcp:project <project>` error: preview failed ``` After: ``` Diagnostics: pulumi:pulumi:Stack (gcp_vm-dev): warning: unable to detect a global setting for GCP Project; Pulumi will rely on per-resource settings for this operation. Set the GCP Project by using: `pulumi config set gcp:project <project>` ```
This should address the error duplication reported in #2285 as suggested by @t0yv0.
We now have a global counter which guard the credentials check and makes sure we only run it once.
For testing, I ran a few programs both with and without errors and it does seem to do the right thing.