-
Notifications
You must be signed in to change notification settings - Fork 116
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 provider diff #1869
Fix provider diff #1869
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
} | ||
if olds["suppressHelmHookWarnings"] != news["suppressHelmHookWarnings"] { | ||
diffs = append(diffs, "suppressHelmHookWarnings") | ||
} |
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.
Can we introspect on the schema and look through the config settings instead of hardcoding/checking these? Seems like that would avoid the whole possibility of falling out of sync 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.
Yeah, I think another option would be iterating the entire struct, and special-casing any we need to handle differently. That would at least give a default diff.
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.
Let me know how you want to proceed. Do the above now or merge this and follow-up. I do worry about the drift so the more we can standardize around schema guiding operations the better. This would also be a great endorsement for common logic living in a library across providers.
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 switched to iterating the keys. Let's figure out how to centralize this logic as a follow up.
More recent provider provider options were not showing up in the diff
8e3ae4f
to
4a25f66
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
1 similar comment
Does the PR have any schema changes?Looking good! No breaking changes found. |
Proposed changes
More recent provider provider options were not showing up in the diff
Related issues (optional)