Skip to content

Migrate rolebindingtemplate controller to kubebuilder#683

Open
mirza-src wants to merge 5 commits intokubebuilder-migrationfrom
kubebuilder
Open

Migrate rolebindingtemplate controller to kubebuilder#683
mirza-src wants to merge 5 commits intokubebuilder-migrationfrom
kubebuilder

Conversation

@mirza-src
Copy link

@mirza-src mirza-src commented Dec 1, 2025

Towards giantswarm/roadmap#3056

rolebindingtemplate

  • rolebinding: Creates RBs based on provided template from RBT custom resource. If a RB is to be created in org-giantswarm namespace, the subjects of the RB is filtered to only include ServiceAccounts that belong in either org-giantswarm or flux-system namespace.

Checklist

  • Update changelog in CHANGELOG.md.
  • If the change introduces new roles, please consider adding user-friendly descriptions to the roles with the annotation giantswarm.io/notes to help explain their purpose and usage.

@mirza-src mirza-src requested a review from a team December 1, 2025 17:25
@fhielpos
Copy link
Member

fhielpos commented Dec 2, 2025

Since this will demand some work, in order to not leave rbac-operator un-releasable, we could point all the work to a new dev or migration branch and add the necessary branch protection rules.

@mirza-src mirza-src changed the base branch from main to kubebuilder-migration December 3, 2025 11:12
@mirza-src mirza-src changed the title Migrate to kubebuilder Migrate rolebindingtemplate controller to kubebuilder Dec 8, 2025
@mirza-src mirza-src marked this pull request as ready for review December 15, 2025 16:23
@mirza-src mirza-src requested a review from a team December 15, 2025 16:23
Copy link
Contributor

@stone-z stone-z left a comment

Choose a reason for hiding this comment

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

This is already remarkably clean, given the scope. Well done 👏

I have some comments, also want to give this another pass and would welcome more team eyes. But overall, this is looking nice

Copy link
Contributor

@stone-z stone-z left a comment

Choose a reason for hiding this comment

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

This is looking good to me now. Are there any trouble spots you anticipate that you want special attention on? Do you feel like the test coverage is adequate?

Comment on lines +205 to +208
if updateErr := r.Status().Update(ctx, template); updateErr != nil {
log.Error(updateErr, errUpdateStatus)
}
return ctrl.Result{}, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on this re-queuing if the status update fails?

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