-
Notifications
You must be signed in to change notification settings - Fork 0
add first version of ClusterProvider documentation #15
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
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.
Thank you for taking the time to write this down! I left some comments for things that we can improve in the future.
| ctrlutils.HasLabelPredicate(clustersv1alpha1.ProviderLabel, providerName), | ||
| // this just checks whether the label exists, independent from its value | ||
| ctrlutils.HasLabelPredicate(clustersv1alpha1.ProfileLabel, ""), |
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 suggest that we split this predicate function into two:
| ctrlutils.HasLabelPredicate(clustersv1alpha1.ProviderLabel, providerName), | |
| // this just checks whether the label exists, independent from its value | |
| ctrlutils.HasLabelPredicate(clustersv1alpha1.ProfileLabel, ""), | |
| ctrlutils.HasLabelValuePredicate(clustersv1alpha1.ProviderLabel, providerName), | |
| // this just checks whether the label exists, independent from its value | |
| ctrlutils.HasLabelPredicate(clustersv1alpha1.ProfileLabel), |
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.
That is probably a good idea. Alternatively, we could also introduce a constant like ArbitraryValue that contains a string which is invalid as label value anyway. Both probably better than an empty string meaning "it doesn't matter".
Independent from how we improve this, I would still first change the implementation and update the documentation afterwards.
| return reconcile.Result{}, nil | ||
| case apiconst.OperationAnnotationValueReconcile: | ||
| log.Debug("Removing reconcile operation annotation from resource") | ||
| if err := ctrlutils.EnsureAnnotation(ctx, myClient, obj, apiconst.OperationAnnotation, "", true, ctrlutils.DELETE); err != nil { |
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.
This might be confusing for new collaborators. Can we simplify it to some thing like this?
| if err := ctrlutils.EnsureAnnotation(ctx, myClient, obj, apiconst.OperationAnnotation, "", true, ctrlutils.DELETE); err != nil { | |
| if err := ctrlutils.RemoveAnnotation(ctx, myClient, obj, apiconst.OperationAnnotation); err != nil { |
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.
Hm, the idea was that the EnsureAnnotation ensures a desired state of the annotation, either that it has a specific value or that it doesn't exist. But I have to admit that the deletion case doesn't look too nice syntactically.
c73e31d
What this PR does / why we need it:
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Release note: