-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing the Microsoft.Insights
Resource Provider Registration
#282
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind explaining how did you discover this bug? If it's already used in any resource then creation of such resource would fail as part of acceptance test I'd assume?
In other words is there any way we could've prevented this bug from appearing and can we do it retroactively?
Context: Resource Provider registration is required to be able to use the API associated with the Resource Provider. During debugging an issue I noticed this Provider was being re-registering multiple times with the debug logs enabled (the provider's init'd multiple times - each time at startup we check to see that the resource providers are registered). This won't affect many users since this is enabled by default, but there's still the potential for this to be de-registered.
We're deliberately catching any errors when registering the Resource Providers, since users may not have permissions to register all of them / they may not all be available in that region, as with Germany and Azure Stack - where they're add-ons. Whilst there is an environment variable to permit resource provider registration to be skipped which would solve the first case - implementing a whitelist is the wrong approach since additional Resource Providers can be installed in some circumstances. After some 🤔 we could probably add a test to try registering the providers, then query again and see if any remain to be registered? |
22908b5
to
bd7ce17
Compare
@radeksimko added a test covering the Resource Provider registration: Result of this test before this change:
Now:
|
260dbd4
to
9713695
Compare
9713695
to
e814616
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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! |
Upon some investigation it appears the Resource Provider API is case-sensitive - this PR fixes the registration for
Microsoft.Insights
to bemicrosoft.insights
so we don't re-register every time the plugin's re-initializedBefore:
After: