-
Notifications
You must be signed in to change notification settings - Fork 456
feat: Cluster-scoped Rollouts support #39
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
Conversation
99c1468 to
25a7148
Compare
jgwest
left a comment
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.
Looks great, thanks Jayendra! See below for requested changes, but it's just minor stuff that I noticed while going the code (some of which was carried over from the original code)
In addition:
-
We should add ClusterRole/Binding to
SetupWithManager, otherwise we won't be notified about changes to these resources -
There are a few places where we are verifying that resources exist (for example, that a
Deploymentexists of nameDefaultArgoRolloutsResourceName), but not the actual contents of those resource (for example, that theDeploymenthas the expected values inside)- I saw a couple spots where this was true, and it would be good to add some quick additional tests to verify resource contents:
- In
validateArgoRolloutManagerResourcesfor E2E tests argorollouts_controller_test.go(I know this came from the original test code, but can likely reuse the same code used for E2E tests here too)
- In
- I saw a couple spots where this was true, and it would be good to add some quick additional tests to verify resource contents:
b1e68ad to
d44b4a9
Compare
|
Addressed all review comments, PTAL. |
17af8b5 to
2ff4355
Compare
|
Yes @jgwest the first comment about duplicate import I had fixed but it seems when I copied some code from another file to rollout_test.go it was added again because of auto import in VSCode. About 2nd comment I removed the error Type and using only one type "Reconciled", but Reason has to have multiple values. I removed duplicate import, PTAL. |
|
@jparsai Perfect, it's likely I misread reason and type, thanks for double checking! |
a3b17ef to
f59332c
Compare
Signed-off-by: Jayendra Parsai <[email protected]>
Signed-off-by: Jonathan West <[email protected]>
Signed-off-by: Jonathan West <[email protected]>
Signed-off-by: Jayendra Parsai <[email protected]>
Signed-off-by: Jonathan West <[email protected]>
24bd76d to
4202d9a
Compare
4ca92e8 to
967a1a7
Compare
|
@jgwest PR is ready for review now, PTAL. |
jgwest
left a comment
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.
Looks great! All that's is just some miscellaneous small items I noticed while reviewing,
967a1a7 to
a261150
Compare
Signed-off-by: Jayendra Parsai <[email protected]>
a261150 to
05a6505
Compare
Signed-off-by: Jonathan West <[email protected]>
What does this PR do / why we need it:
This PR is to add support for cluster scoped Rollouts. Currently Rollouts controller can reconcile CR created in same namespace, this PR has changes which would allow Rollouts controller to reconcile CR created in other namespaces as well.
There are few limitations for creating Cluster scoped Rollouts controller, please check original issue created for this feature.
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
#20
How to test changes / Special notes to the reviewer:
RolloutManagerCR and change scope to namespace or cluster usingSpec.NamespaceScopedfield ofRolloutManager.RolloutsCR ifRolloutManageris cluster scoped and it should be reconciled.