Improve the provider initialization #322
Conversation
|
@tombuildsstuff would be great if you could review this PR so we can try to land it before the next release. Thx! |
The `registerAzureResourceProvidersWithSubscription` function is being called exactly once while executing the `providerConfigure` func. In turn the `providerConfigure` func is also called exactly once while executing a Terraform command (e.g. plan, apply). So when executing TF commands from the commandline there is absolutely no added value for using a `sync.Once` as the registration call is only being executed once anyway. If on the other hand you use the TF packages in a long running microservice process (like we do), having this `sync.Once` actually causes issues when using multiple different (and regularly changing) provider credentials (as only the first used credentials will register the providers). So since it adds nothing, but does cause issues in specific setups, I refactored the provider to not use the `sync.Once` anymore.
Nice if you make multiple validation or plan calls in a row from a microservice, before actually making the apply call. With this option we can skip this expensive HTTP call as we already know the credentials are valid.
There was a problem hiding this comment.
Hey @svanharmelen
Thanks for this PR - apologies for the delay in reviewing this!
I've taken a look through and I'm going to run the acceptance tests against this branch to confirm this won't affect those running in parallel - but this otherwise LGTM :)
Thanks!
| var err error | ||
| providerRegistrationOnce.Do(func() { |
There was a problem hiding this comment.
I'm not 100% sure of the original need for this - I'm assuming it's needed for the acceptance tests (which run in parallel) - to verify this I'll run the acceptance tests against this branch shortly to confirm
There was a problem hiding this comment.
Acceptance tests passed - so looks like we're good here :)
There was a problem hiding this comment.
Cool, good the hear that! And thanks for the review 👍
|
Thanks @tombuildsstuff! |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
The
registerAzureResourceProvidersWithSubscriptionfunction is being called exactly once while executing theproviderConfigurefunc.In turn the
providerConfigurefunc is also called exactly once while executing a Terraform command (e.g. plan, apply).So when executing TF commands from the commandline there is absolutely no added value for using a
sync.Onceas the registration call is only being executed once anyway.If on the other hand you use the TF packages in a long running microservice process (like we do), having this
sync.Onceactually causes issues when using multiple different (and regularly changing) provider credentials (as only the first used credentials will register the providers).So since it adds nothing, but does cause issues in specific setups, we refactored the provider to not use the
sync.Onceanymore.Additionally we added an option to skip the credential validation call. This is nice if you make multiple validation or plan calls in a row from a microservice so you can skip the expensive HTTP call (as you already know the credentials are valid).