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

add karmada.io/system label to created clusterrole+bindings #5281

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Jul 30, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

similar to namespaces cluster-roles need to have an identifier so we can find them all (without having to guess based on names)
and allow-list them in our "are you allowed to create cluster-roles" auditing tooling

Does this PR introduce a user-facing change?:

- add karmada.io/system=true label to internally created karmada cluster-roles and cluster-role-bindings

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 30, 2024
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 30, 2024
@grosser
Copy link
Contributor Author

grosser commented Jul 30, 2024

cc @XiShanYongYe-Chang

@XiShanYongYe-Chang
Copy link
Member

Hi @grosser, the lint is failed.

@grosser grosser force-pushed the grosser/role-labels branch from 14414ec to 4fd67ed Compare July 31, 2024 16:08
@grosser
Copy link
Contributor Author

grosser commented Jul 31, 2024

thx, fixed!

@XiShanYongYe-Chang
Copy link
Member

The UT has a falling state.

@grosser
Copy link
Contributor Author

grosser commented Aug 1, 2024

https://github.com/karmada-io/karmada/blob/master/CONTRIBUTING.md#contributor-workflow only has make test, but I need to run a single file and ideally a single test, otherwise this takes forever :/

@grosser
Copy link
Contributor Author

grosser commented Aug 1, 2024

also make test does not show colors :/

then at least it prints the package name, but there should be instructions to run the package with go test

and the test prints this giant karmada font that it should not do

go test github.com/karmada-io/karmada/pkg/karmadactl/cmdinit/utils

------------------------------------------------------------------------------------------------------
 █████   ████   █████████   ███████████   ██████   ██████   █████████   ██████████     █████████
░░███   ███░   ███░░░░░███ ░░███░░░░░███ ░░██████ ██████   ███░░░░░███ ░░███░░░░███   ███░░░░░███

@grosser
Copy link
Contributor Author

grosser commented Aug 1, 2024

and the failed tests don't point to the line number that failed either 😞

@grosser grosser force-pushed the grosser/role-labels branch 2 times, most recently from 159fc65 to 0fa584b Compare August 1, 2024 01:40
@grosser
Copy link
Contributor Author

grosser commented Aug 1, 2024

fixed the tests hopefully 🤞

@grosser grosser force-pushed the grosser/role-labels branch from 0fa584b to f4df03f Compare August 1, 2024 01:46
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 30.00000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 28.24%. Comparing base (145a67e) to head (f4df03f).
Report is 2 commits behind head on master.

Files Patch % Lines
operator/pkg/karmadaresource/rbac/rbac.go 0.00% 8 Missing ⚠️
...controllers/unifiedauth/unified_auth_controller.go 0.00% 6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5281   +/-   ##
=======================================
  Coverage   28.23%   28.24%           
=======================================
  Files         632      632           
  Lines       43762    43774   +12     
=======================================
+ Hits        12357    12363    +6     
- Misses      30504    30511    +7     
+ Partials      901      900    -1     
Flag Coverage Δ
unittests 28.24% <30.00%> (+<0.01%) ⬆️

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.

@XiShanYongYe-Chang
Copy link
Member

@XiShanYongYe-Chang
Copy link
Member

Hi @grosser thanks for your contribution!

We now have a series of prs that add Karmada system or manage labels to resources. Can you help create an issue to organize all the prs in the series, so that we can have a general record of this and facilitate later retrieval.

Also, do we have similar resources to work with?

@grosser
Copy link
Contributor Author

grosser commented Aug 1, 2024

is this what you were looking for #5288 ?

"Also, do we have similar resources to work with?"
do you mean "are there more resources that needs labels" ?
I'm not sure, I did all that were getting flagged for us, there might be some more like configmap/secret that we don't have as much auditing for as for pods/clusterroles.

@XiShanYongYe-Chang
Copy link
Member

Thanks @grosser

If more resources need to be modified, you can modify the track issue accordingly.

Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

Thanks~
/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2024
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: XiShanYongYe-Chang

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

The pull request process is described here

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2024
@karmada-bot karmada-bot merged commit 6b87604 into karmada-io:master Aug 2, 2024
12 checks passed
@RainbowMango RainbowMango added this to the v1.11 milestone Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants