Skip to content

Clean up ScyllaCluster GET ClusterRole #2315

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

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

mflendrich
Copy link
Collaborator

Description of your changes:
Deletes the GET verb permission from the member ClusterRole because Operator >=1.16 no longer needs it.

Which issue is resolved by this Pull Request:
Resolves #2141

Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@scylla-operator-bot scylla-operator-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 10, 2025
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mflendrich

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

@scylla-operator-bot scylla-operator-bot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2025
@mflendrich
Copy link
Collaborator Author

/test all

@mflendrich
Copy link
Collaborator Author

/kind cleanup
/priority important-soon

@scylla-operator-bot scylla-operator-bot bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 10, 2025
@mflendrich mflendrich marked this pull request as ready for review January 10, 2025 16:35
@mflendrich mflendrich changed the title [WIP] clean up ScyllaCluster GET ClusterRole Clean up ScyllaCluster GET ClusterRole Jan 10, 2025
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2025
Copy link
Member

@rzetelskik rzetelskik left a comment

Choose a reason for hiding this comment

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

Please remove the prefixes from the commit subject lines (explained internally, sadly we don't have the contributing guide now).
The 2nd commit subject line should also use the imperative mood - we usually name the make update commits as Update generated/Update autogenerated.
Other than that it looks good to me, thanks @mflendrich

@mflendrich
Copy link
Collaborator Author

@rzetelskik commits reworded, please look now

@mflendrich
Copy link
Collaborator Author

/retest

1 similar comment
@mflendrich
Copy link
Collaborator Author

/retest

@rzetelskik
Copy link
Member

@mflendrich almost there, but the commit subject line should answer the following sentence If applied, this commit will <your subject line here> (ref #2130 (comment)). So "Run make update" is not quite what we're aiming for.

Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

lgtm, except commit naming as per rzetelskik comments.
/assign rzetelskik

This is simply a run of `make update` following the previous commit.
@mflendrich
Copy link
Collaborator Author

@rzetelskik done, please look again if this looks right.

Process feedback: When a review expects rewording (of a commit message or something else), it would make my life easier (and make the review comment more actionable, and better communicate the reviewer's expectation) to see a direct wording suggestion.

@mflendrich mflendrich requested a review from rzetelskik January 13, 2025 15:04
@rzetelskik
Copy link
Member

/lgtm

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2025
@scylla-operator-bot scylla-operator-bot bot merged commit 1d88107 into scylladb:master Jan 13, 2025
12 checks passed
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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove ScyllaCluster GET permission from member role
3 participants