fix: should create CRB not RB + add ControlledBy() for MetaOptions

opendatahub-io / opendatahub-operator

Open Data Hub operator to manage ODH component integrations

https://opendatahub.io

Apache License 2.0

59 stars 127 forks source link

fix: should create CRB not RB + add ControlledBy() for MetaOptions #1119

Closed zdtsw closed 1 month ago

zdtsw commented 1 month ago

fix resource type: it should be a clusterrolebinding than a rolebiding, if roleref is set to clusterrole not role.
replace createDefaultRoleBinding() with CreateOrUpdateClusterRoleBinding()
ref package from ctrl to controllerutil
update testcase

Description

How Has This Been Tested?

local build: quay.io/wenzhou/opendatahub-operator-catalog:v2.14.77-715

Screenshot or short clip

Merge criteria

[ ] You have read the contributors guide.
[ ] Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
[ ] Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
[ ] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
[ ] The developer has manually tested the changes and verified that the changes work

openshift-ci[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please ask for approval from zdtsw. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

zdtsw commented 1 month ago

~We have pkg/cluster/roles.go - can't we unify the code base? Move new funcs to this file and reuse if anything is possible to be reused?~

Ok I shouldn't review from mobile. Ignore this comment.

ignored -- replied from computer

adelton commented 1 month ago

Do we have some issue describing what problems the current approach causes?

I assume the code has been there as is for a while, so this type of change seems a bit ... sudden.

zdtsw commented 1 month ago

i am gonna close this ticket, based on these:

we do set namespace for the "opendatahub" RoleBinding and use RoleRef with ClusterRole, this is expected. not a mistake from past
there is no need to create a new func CreateOrUpdateRoleBinding() just for the sake to replace the current createDefaultRoleBinding() which is functional.