search

argoproj-labs / argo-rollouts-manager

Kubernetes Operator for Argo Rollouts controller.
https://argo-rollouts-manager.readthedocs.io/en/latest/
Apache License 2.0
84 stars 255 forks source link

feat: Cluster-scoped Rollouts support #39

Closed jparsai closed 2 months ago

jparsai commented 4 months ago

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 #? https://github.com/argoproj-labs/argo-rollouts-manager/issues/20

How to test changes / Special notes to the reviewer:

  1. Create RolloutManager CR and change scope to namespace or cluster using Spec.NamespaceScoped field of RolloutManager.
  2. Create Rollouts CR if RolloutManager is cluster scoped and it should be reconciled.
  3. Try combinations of cluster and namespace scoped Rollouts controller to confirm it works as expected and doesn't allow unsupported scenarios.
jparsai commented 4 months ago

Addressed all review comments, PTAL.

jgwest commented 4 months ago

@jparsai did you miss pushing a commit, by chance? I started re-reviewing by checking the first two comments , and it seems like both are still present in the code.

jparsai commented 4 months ago

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. Type=Reconciled | Status = true | Reason=Success | Message = "" Type=Reconciled | Status = false | Reason=ErrorOccurred | Message = "Some error message"

I removed duplicate import, PTAL.

jgwest commented 4 months ago

@jparsai Perfect, it's likely I misread reason and type, thanks for double checking!

jparsai commented 3 months ago

@jgwest PR is ready for review now, PTAL.