-
Notifications
You must be signed in to change notification settings - Fork 10
Add rollback controller boilerplate #436
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
Add rollback controller boilerplate #436
Conversation
4639c2d to
42743f8
Compare
cmd/rollback-manager/main.go
Outdated
|
|
||
| manager, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ | ||
| LeaderElection: commonOptions.ManagerLeaderElection, | ||
| LeaderElectionID: "deploy.crds.gocardless.com", |
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 think this has to be rollback.deploy.crds.gocardless.com as the documentation states:
LeaderElectionID determines the name of the resource that leader election will use for holding the leader lock.
I don't think this is as applicable to us, as we don't plan to run multiple replicas of the controller as of yet, but it might be useful if we decide to do it in the future.
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.
Right, gotcha. I'm assuming we should also change the release controller election ID to release.deploy.crds.gocardless.com then.
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've updated both now.
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
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'm not sure what the mechanism would be, but wouldn't the controller also need to update the Release?
I can think of a scenario where a rollback has happened, and we want to release status to indicate that it has occurred. I'm pretty sure we wouldn't want the rollback controller to update the status field of the release resource directly, but maybe we could manage that through annotations on the releases.
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'm not (yet) convinced we need to update an existing Release by the rollback controller. However, if we decide to do so, maybe updating a specific (rollback controller owned) condition would be suitable?
However, maybe a better way would be to somehow inject this information through deployment mechanism during deployment, so it would be present during the creation of the Release resource? We already have the message field, so I assume there was some idea of how this could get propagated?
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.
Yep, I think I agree with @0x0013 - if we want to reflect the rollback in the Release resource I'd prefer to include that information in the ArgoCD notification and have the release-recorder inject it somehow than have both controllers managing the Release resource, even if indirectly via annotations.
Co-authored-by: Emil Lozev <elozev@gocardless.com>
Adding the rollback controller boilerplate.