refactor controllers #48
Conversation
Signed-off-by: Bryce Palmer <[email protected]>
Signed-off-by: Bryce Palmer <[email protected]>
Signed-off-by: Bryce Palmer <[email protected]>
everettraven
requested review from
laxmikantbpandhare,
awgreene,
jmrodri and
joelanford
and removed request for
laxmikantbpandhare and
awgreene
Signed-off-by: Bryce Palmer <[email protected]>
Signed-off-by: Bryce Palmer <[email protected]>
| return &unstructured.Unstructured{Object: uMap}, nil | ||
| } | ||
|
|
||
| func (r *ScopeInstanceReconciler) patchBinding(ctx context.Context, binding client.Object) error { |
There was a problem hiding this comment.
patchBinding seems good as well.
| binding, | ||
| client.Apply, | ||
| client.FieldOwner(siCtrlFieldOwner), | ||
| client.ForceOwnership) | ||
| } |
There was a problem hiding this comment.
deleteBindings seems like it might fail between the list and delete.
There was a problem hiding this comment.
sigh, the comment says just that. And um it's not even part of this PR.
| requests = append(requests, request) | ||
| } | ||
|
|
||
| return | ||
| } | ||
|
|
||
| // getClusterRoleBindingForClusterRoleTemplate will create a ClusterRoleBinding from a ClusterRoleTemplate, ScopeInstance, and ScopeTemplate | ||
| func (r *ScopeInstanceReconciler) getClusterRoleBinding(cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) *rbacv1.ClusterRoleBinding { |
There was a problem hiding this comment.
getClusterRoleBinding looks good, cleans up the rest of the code a lot.
| } | ||
|
|
||
| // getRoleBindingForClusterRoleTemplate will create a ClusterRoleBinding from a ClusterRoleTemplate | ||
| func (r *ScopeInstanceReconciler) getRoleBinding(cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate, namespace string) *rbacv1.RoleBinding { |
There was a problem hiding this comment.
getRoleBinding looks good too. Also cleans up the rest of the code.
everettraven
changed the title
Signed-off-by: Bryce Palmer <[email protected]>
|
/lgtm |
|
/hold There are still a few things I want to look into changing and seeing if it will work and make the code look cleaner |
openshift-ci
bot
added
the
do-not-merge/hold
to just return unstructured.Unstructured instead of dealing with applyconfigurations Signed-off-by: Bryce Palmer <[email protected]>
Signed-off-by: Bryce Palmer <[email protected]>
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
Signed-off-by: Bryce Palmer <[email protected]>
|
Created an issue to update the reconcile comment. #60 |
There was a problem hiding this comment.
Okay really great work. Some minor tweaks. Some of the comments are really long, let's narrow those to about 80 characters to make them easier to read. I supplied suggestions in the PR to help with that.
I left comments about the 2 patchConfig....
There might be some other comments that I forgot to summarize in this comment.
|
The one bigger nit I have is that we keep passing ScopeTemplate and ScopeInstances all the way down through all of the methods. This makes the methods harder to understand IMO. For example, in
But this isn't really doable because the embedded methods called from So there's no action needed on this nit, but it feels like we should be able to tighten our method signatures to only use what they need and not have to pass entire objects all the way down the stack. Maybe there isn't an efficient way to do it. But my spidey senses are tingly. |
Signed-off-by: Bryce Palmer <[email protected]>
Signed-off-by: Bryce Palmer <[email protected]>
Signed-off-by: Bryce Palmer <[email protected]>
Signed-off-by: Bryce Palmer <[email protected]>
|
/lgtm |
Co-authored-by: Alexander Greene <[email protected]>
Co-authored-by: Alexander Greene <[email protected]>
Signed-off-by: Bryce Palmer <[email protected]>
Description of the change
controllers/scopeinstance_controller.goandcontrollers/scopetemplate_controller.goto align more with the implementation that rukpak has.Motivation for the change
ScopeInstanceallClusterRoleson the cluster are deleted #26labels.Requirementsused inclient.ListOptionsto only deleteClusterRoles and(Cluster)RoleBindings created by the oria-operator #28