Skip to content

ACM-22604 Initial clusterRoleBindings feature - #62

Merged
openshift-merge-bot[bot] merged 3 commits into
open-cluster-management-io:mainfrom
fxiang1:feng-multicrb
Jul 30, 2025
Merged

ACM-22604 Initial clusterRoleBindings feature#62
openshift-merge-bot[bot] merged 3 commits into
open-cluster-management-io:mainfrom
fxiang1:feng-multicrb

Conversation

@fxiang1

@fxiang1 fxiang1 commented Jul 29, 2025

Copy link
Copy Markdown
Collaborator

https://issues.redhat.com/browse/ACM-22604

  • Add new field clusterRoleBindings (plural) optional
  • Kept old field clusterRoleBinding (singular) optional for backwards compatibility
  • If both fields exist clusterRoleBindings will override clusterRoleBinding

Signed-off-by: fxiang1 <fxiang@redhat.com>
@openshift-ci
openshift-ci Bot requested review from mikeshng and xiangjingli July 29, 2025 14:40
@fxiang1 fxiang1 changed the title ACM-22604 Initial clusterRoleBindings feature [WIP] ACM-22604 Initial clusterRoleBindings feature Jul 29, 2025
Signed-off-by: fxiang1 <fxiang@redhat.com>
@codecov

codecov Bot commented Jul 29, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.61%. Comparing base (1678382) to head (4a5b60d).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   68.24%   69.61%   +1.37%     
==========================================
  Files           2        2              
  Lines         381      418      +37     
==========================================
+ Hits          260      291      +31     
- Misses        107      111       +4     
- Partials       14       16       +2     
Flag Coverage Δ
unit 69.61% <ø> (+1.37%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fxiang1 fxiang1 changed the title [WIP] ACM-22604 Initial clusterRoleBindings feature ACM-22604 Initial clusterRoleBindings feature Jul 29, 2025
@fxiang1

fxiang1 commented Jul 30, 2025

Copy link
Copy Markdown
Collaborator Author

/uncc @xiangjingli

@openshift-ci
openshift-ci Bot removed the request for review from xiangjingli July 30, 2025 13:38
@fxiang1

fxiang1 commented Jul 30, 2025

Copy link
Copy Markdown
Collaborator Author

@mikeshng Please review when you have some cycles. Thanks!

@mikeshng

Copy link
Copy Markdown
Member

@mikeshng Please review when you have some cycles. Thanks!

Thanks. Will do! It still have the do not merge hold tag that's why I didn't start reviewing it yet. Removing it.

/unhold

Subjects: subjects,
// We make the behaviour consistent with how subject and subjects are handled in ClusterRoleBinding
// subjects will just override subject if both are provided
if clusterPermission.Spec.ClusterRoleBindings == nil || len(*clusterPermission.Spec.ClusterRoleBindings) == 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This if block is confusing and I think unnecessary. To deal with the spec crbs and backward compatible spec crb, we should add the handling spec crbs logic first. Like:

If crbs exists then do crbs stuffs, so now the return var "clusterRoleBindings" should not be empty anymore.

Only if return var "clusterRoleBindings" is empty and spec crb exist then do crb stuffs

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok how about the latest change? I tried a different AI prompt.

Signed-off-by: fxiang1 <fxiang@redhat.com>

@mikeshng mikeshng left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/approve

/lgtm

I still prefer having the plural block to come first but it's not a big deal.

@openshift-ci

openshift-ci Bot commented Jul 30, 2025

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fxiang1, mikeshng

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot
openshift-merge-bot Bot merged commit c8c8afe into open-cluster-management-io:main Jul 30, 2025
8 checks passed
@fxiang1
fxiang1 deleted the feng-multicrb branch July 30, 2025 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants