feat: override triggers rollout #993
Conversation
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pkg/controllers/rollout/controller.go:713
- The error message should mention 'label' instead of 'annotation' for consistency.
klog.ErrorS(controller.NewUnexpectedBehaviorError(fmt.Errorf("invalid annotation value %s : %w", fleetv1beta1.IsLatestSnapshotLabel, err)),
|
|
||
| // handleClusterResourceOverrideSnapshot parse the clusterResourceOverrideSnapshot label and enqueue the CRP name associated | ||
| // with the clusterResourceOverrideSnapshot if set. | ||
| func handleClusterResourceOverrideSnapshot(o client.Object, q workqueue.RateLimitingInterface) { |
There was a problem hiding this comment.
is there a way to combine these two functions? We will have RP and CRP co exist all over the place soon.
There was a problem hiding this comment.
we could, but need to rely on the if else condition to handle some cases based on the type, including logging, which hurts our readability.
For the current case, prefer to do the copy & paste.
There was a problem hiding this comment.
is there a way to use generic templating?
There was a problem hiding this comment.
it may be workable, either using https://google.github.io/styleguide/go/decisions#generics or interface.
zhiying-lin
force-pushed
the
override-trigger-rollout
branch
from
bd5a1b5 to
9dc98fe
Compare
2 participants
Description of your changes
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
added e2e tests
Special notes for your reviewer