Skip to content
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

remove toolchaincluster resources creation #107

Merged

Conversation

mfrancisc
Copy link
Contributor

@mfrancisc mfrancisc commented Apr 18, 2024

@MatousJobanek
Copy link
Collaborator

How about the ServiceAccount creation?

cat <<EOF | oc apply ${OC_ADDITIONAL_PARAMS} -f -
apiVersion: v1
kind: ServiceAccount
metadata:
name: ${SA_NAME}
namespace: ${OPERATOR_NS}
EOF

shouldn't we drop that too?

@mfrancisc
Copy link
Contributor Author

How about the ServiceAccount creation?

@MatousJobanek I tried but I think we need to get rid of

SA_TOKEN=$(oc create token ${SA_NAME} --duration 87600h -n ${OPERATOR_NS} ${OC_ADDITIONAL_PARAMS})
. I see it's expected there at least and was causing a failure.

@mfrancisc
Copy link
Contributor Author

I think we will remove that once we create the ToolchainCluster from the controller as well, right ?

@MatousJobanek
Copy link
Collaborator

it's not about creating ToolchainCluster CR, but rather about timing when the SA is created.
These SA should be already created using the new ToolchainCluster_resources controller, right? We maybe call the script too early when the SA is not there yet. Execution of the script (and of the register-member command) relies on the existence of the SA we want to get the token for.
Copy of the secret will be always a manual step, so we should wait until the SA is present.

Comment on lines -29 to -31
if [[ -n `oc get rolebinding ${SA_NAME} 2>/dev/null` ]]; then
oc delete rolebinding ${SA_NAME} -n ${OPERATOR_NS} ${OC_ADDITIONAL_PARAMS}
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we update the ToolchainCluster_resources controller to delete the rolebinding before applying the templates before we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @rajivnathan !

TBH I wasn't aware of this limitation - and this might complicate things a bit since the resource controller is not aware of which resources is creating, it should be generic.

Maybe @MatousJobanek might know better if this is still required ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deletion part was there just to make sure that the RoleBinding can be updated also in the cases when we would change the name of the Role that is referenced there - you know, it's not possible to update roleRef for an already existing RoleBinding.
I guess that this is from the time when we were doing many changes in the content of the script/resources.

That being said, I think that it's fine to keep it as it is in the Toolchaincluster resources controller 👍 we can change it later in the case when we would need to update the roleRef.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok , thanks for the background . Thus we can avoid the deletion part in the controller atm.

CLUSTER_ROLE_NAME=${SA_NAME}-${OPERATOR_NS}-toolchaincluster
# we need to delete the binding since we cannot change the roleRef of the existing binding
if [[ -n `oc get ClusterRoleBinding ${CLUSTER_ROLE_NAME} ${OC_ADDITIONAL_PARAMS} 2>/dev/null` ]]; then
oc delete ClusterRoleBinding ${CLUSTER_ROLE_NAME} ${OC_ADDITIONAL_PARAMS}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the same reason - to be able to update ClusterRole name in the roleRef section

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@mfrancisc mfrancisc merged commit ecd861c into codeready-toolchain:master Jun 3, 2024
@mfrancisc mfrancisc deleted the removetoolchainclusterres branch June 3, 2024 13:33
MatousJobanek pushed a commit to MatousJobanek/toolchain-cicd that referenced this pull request Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants