Skip to content
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

Fixes issues with ConfigMap annotations #133

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

rg2011
Copy link
Contributor

@rg2011 rg2011 commented Mar 6, 2022

This PR addresses two issues with how ConfigMap annotations are handled:

Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me! Thanks for working on this 👍 I'm not too sure about introducing another dependency to be used only in a single location though, at least not if it doesn't mean a huge improvement where it's used, and I can't see that being the case here. If you feel that kube-mgmt as a whole would benefit from that library, I'd prefer to see another PR introducing that dependency, along with applying it where it results in an improvement. Just my two cents though :)

@rg2011 rg2011 force-pushed the fix/issue-90 branch 2 times, most recently from 138ced1 to 5138f0f Compare March 6, 2022 20:54
@rg2011
Copy link
Contributor Author

rg2011 commented Mar 6, 2022

The change looks good to me! Thanks for working on this 👍 I'm not too sure about introducing another dependency to be used only in a single location though, at least not if it doesn't mean a huge improvement where it's used, and I can't see that being the case here. If you feel that kube-mgmt as a whole would benefit from that library, I'd prefer to see another PR introducing that dependency, along with applying it where it results in an improvement. Just my two cents though :)

Hi, thanks for reviewing! Yes, for this use case hashicorp/multierror can be easily replaced with a small error list. I have removed the dependency and rebased.

@anderseknert
Copy link
Member

Splendid! Thanks @rg2011 👍

@rg2011
Copy link
Contributor Author

rg2011 commented Mar 7, 2022

Hi @anderseknert , would you please merge the PR? or is there something else I'd need to do?

@anderseknert
Copy link
Member

Hey @rg2011 ! Nothing else you need to do, sorry for the delay... was just giving some time for others to review, but let's get it merged.

@anderseknert anderseknert merged commit cf978af into open-policy-agent:master Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants