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

fix: add missing RBAC permissions to autoscaler chart #4154

Merged
merged 2 commits into from
Jul 8, 2021
Merged

fix: add missing RBAC permissions to autoscaler chart #4154

merged 2 commits into from
Jul 8, 2021

Conversation

MarcusNoble
Copy link
Contributor

Fixes #4114

Added RBAC permissions to watch, list and get csidrivers and csistoragecapacities

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 21, 2021
@k8s-ci-robot k8s-ci-robot requested review from bskiba and mwielgus June 21, 2021 06:30
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 21, 2021
@MarcusNoble
Copy link
Contributor Author

/assign @gjtempleton

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2021
@gjtempleton
Copy link
Member

Sorry for the delay in reviewing.
Thanks for the PR!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gjtempleton, MarcusNoble

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit 23fdf63 into kubernetes:master Jul 8, 2021
@stevehipwell
Copy link
Contributor

I think the chart version here 9.10.01 is a typo and could cause issues due to not being a valid SemVer. This should probably be resolved before updating the gh-pages branch with the chart releases, I'm happy to open a PR to raise this to 9.10.2.

@gjtempleton
Copy link
Member

@stevehipwell Good catch, you're 100% correct.

If you could raise that PR that would be great. I've stuck a hold on the PR to update the index.yaml for now.
I'll have a look into why the chart version not being SemVer didn't lead to a failed CI run.

@MarcusNoble
Copy link
Contributor Author

Ah yes! Good catch indeed. Looks like I made a typo when handling the merge conflict.

@stevehipwell
Copy link
Contributor

I've added PR #4183.

@gjtempleton
Copy link
Member

After a bit of digging, found why this slipped through linting. It seems both chart-testing and helm's semver functionalities use https://github.com/Masterminds/semver#parsing-semantic-versions and use the NewVersion rather than StrictNewVersion functionality.

With a bit of testing in a cluster this non-compliant version will still successfully install, as it seems all of the code dealing with SemVer handles these versions in the same way.

@stevehipwell
Copy link
Contributor

That's my understanding too, but other tools using StrictNewVersion will error. There is also a precedence for Helm itself to start erroring as it does from v3 with versions not even close to SemVer (the official Flink repo has this issue).

dkoshkin added a commit to dkoshkin/autoscaler that referenced this pull request Aug 17, 2021
piotrnosek pushed a commit to piotrnosek/autoscaler that referenced this pull request Nov 30, 2021
MaxRink pushed a commit to MaxRink/autoscaler that referenced this pull request Feb 14, 2022
tim-smart pushed a commit to arisechurch/autoscaler that referenced this pull request Nov 22, 2022
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autoscaler chart is missing some rules
4 participants