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

chore: cleanup reconcilation of roles, role bindings, service accounts #605

Merged

Conversation

ishitasequeira
Copy link
Collaborator

@ishitasequeira ishitasequeira commented Apr 11, 2022

Signed-off-by: ishitasequeira ishiseq29@gmail.com

What type of PR is this?
/kind chore

What does this PR do / why we need it:
The reconciliation logic for Roles, Role Bindings and Service Accounts is confusing and has a lot of redundant code. This PR tries to cleanup the reconciliation logic and reduce number of calls to API server.

Have you updated the necessary documentation?

  • Documentation update is required by this PR.
  • Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #? GITOPS-1854

How to test changes / Special notes to the reviewer:
Execute below commands on separate windows of terminal

  1. make install run
  2. make test

@ishitasequeira ishitasequeira marked this pull request as ready for review April 11, 2022 23:07
@jaideepr97
Copy link
Collaborator

jaideepr97 commented Apr 18, 2022

LGTM, thanks @ishitasequeira !

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>
Copy link
Collaborator

@iam-veeramalla iam-veeramalla left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @ishitasequeira

Just a suggestion, going forward can you please update the PR with fields like

What does this PR do / why we need it:
How to test the changes

I can see that you have provided the JIRA ticket (which has details) but I don't mind duplication :)

@iam-veeramalla iam-veeramalla merged commit 68e1197 into argoproj-labs:master Apr 21, 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.

3 participants