-
Notifications
You must be signed in to change notification settings - Fork 193
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
Fix use of API Version in reconciler #2419
Conversation
|
||
apiVersion, err := genruntime.GetAPIVersion(metaObject, scheme) | ||
if err != nil { | ||
panic(err) |
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.
I think we should avoid panic
in the reconciler codepaths as much as we can? I know it makes it more awkward to use but can we return err
here instead?
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.
Agree with @matthchr here.
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.
I was on the fence; originally wrote it with errors and then changed to use a panic to reduce boilerplate.
Changed it back.
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! Thanks for making that err change
Closes #[issue number]
What this PR does / why we need it:
The API version associated with a resource is the one that should always be used when making ARM REST API calls. This wasn't the case, as documented in #2416
Fixes #2416
Special notes for your reviewer:
Thanks to @matthchr for helping to track down the issue.
How does this PR make you feel: