-
Notifications
You must be signed in to change notification settings - Fork 411
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
✨ rbac: deduplicate having the same groups, resourceNames, URLs and Verbs #937
✨ rbac: deduplicate having the same groups, resourceNames, URLs and Verbs #937
Conversation
dc44d68
to
56a3b24
Compare
56a3b24
to
2220e4d
Compare
@@ -52,7 +52,7 @@ var _ = Describe("ClusterRole generated by the RBAC Generator", func() { | |||
Expect(err).NotTo(HaveOccurred()) | |||
|
|||
By("parsing the desired YAML") | |||
for i, expectedRoleBytes := range bytes.Split(expectedFile, []byte("\n---\n"))[1:] { | |||
for i, expectedRoleBytes := range bytes.Split(expectedFile, []byte("\n---\n")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this change allows to use the directly generated output as golden file. The [1:]
is not necessary anymore because the document starts with ---\n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few smaller findings and some ideas to test more before merge
ead6212
to
73b2aad
Compare
Example PR: kubernetes-sigs/cluster-api#10612 |
Thx! CAPI PR looks entirely as expected Really nice optimization, Roles look much cleaner now /lgtm |
LGTM label has been added. Git tree hash: 3578a43773ef68570ca017fb76ea52b800335039
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrischdi, sbueringer 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 |
Just tested 0.16.1 with this change and there's 1 little quirk: If I have 2 different but logically identical codemarkers:
and
I'll get the following generated with 0.16.1:
The workaround is to use the same apigroup everywhere. |
Ah interesting. Can you open an issue for that, please? We should be able to improve this further :) (cc @chrischdi, maybe you have some time to look into it (let's also add test coverage for this case)) |
|
This improves the output of controller-gens rbac generator to further deduplicate the output.
It now:
The first commit only adds test data.
The second commit adds the change for deduplicating based on resources
The third commit deduplicates on apiGroups.