-
Notifications
You must be signed in to change notification settings - Fork 118
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
Skip API operations for diff run #793
Conversation
Hmm, not sure why the new test
The other two tests ( |
Hey @vixus0! Thank you so much for the PR. Like you mentioned the other 2 tests are failing on develop as well, most probably because of some changes in the latest version of Kubernetes. I will fix them as soon as I get some time. |
Yeah, I can consistently run the test on my machine. It seems in CI the initial
|
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 re ran the tests and the test seems to be passing now (except the other 2, I have created a PR to fix them), I am guessing it was a one time thing, although I am not sure how that error could possible come.
Left some comments.
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.
Looking really good now 🚀
Just left some minor comments.
The PR to fix the other 2 tests is merged, so you can rebase on those changes now for the tests to pass. |
Diff run should be usable when the authorised RBAC role doesn't have write permissions for resources in the cluster. Diff run on app creation used the dry run feature of the Kubernetes API, to avoid persisting kapp ConfigMap changes. However this still checks RBAC permissions, and errored if the role did not have write permissions for ConfigMaps. The kapp ConfigMap was also being updated when doing a diff run for an existing app. This skips the API operations for updating the ConfigMap when doing a diff run. Fixes #791. Signed-off-by: Anshul Sirur <anshul@vixus0.dev>
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! Thank you so much for the PR @vixus0 🚀
What this PR does / why we need it:
Diff run should be usable when the authorised RBAC role doesn't have
write permissions for resources in the cluster.
Diff run on app creation used the dry run feature of the Kubernetes
API, to avoid persisting kapp ConfigMap changes. However this still
checks RBAC permissions, and errored if the role did not have write
permissions for ConfigMaps.
The kapp ConfigMap was also being updated when doing a diff run for an
existing app.
This skips the API operations for updating the ConfigMap when doing a
diff run.
Which issue(s) this PR fixes:
Fixes #791
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: