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

Refine the minimum permissions needed by Velero in README #133

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

ywk253100
Copy link
Contributor

Refine the minimum permissions needed by Velero in README

Signed-off-by: Wenkai Yin(尹文开) [email protected]

@ywk253100
Copy link
Contributor Author

Fixes vmware-tanzu/velero#3154

@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2022

Codecov Report

Merging #133 (7ec8561) into main (f287698) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #133   +/-   ##
=======================================
  Coverage   12.64%   12.64%           
=======================================
  Files           4        4           
  Lines         609      609           
=======================================
  Hits           77       77           
  Misses        528      528           
  Partials        4        4           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f287698...7ec8561. Read the comment docs.

a-mccarthy
a-mccarthy previously approved these changes Apr 7, 2022
Copy link

@a-mccarthy a-mccarthy left a comment

Choose a reason for hiding this comment

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

@ywk253100 I think these changes look great! I am not familiar with Azure permissions, so i'd also recommend getting a second review from another maintainer/community member to have a second pair of eyes on the tech pieces of this PR.

I'm approving the changes, but have one question/suggestion. Is the information in the removed Storage account, snapshot and disk management sections still valid? I like the way the required permissions are listed out with bullet points. I think it makes the information very clear to read and scan for. If it's still correct, i'd suggest keep it in the doc right before the improved section on how to create roles and assign these permissions that you added.

You have included the information within the new content you added, so it's maybe not necessary, but it might be more clear to have it both ways for users. If it's no longer valid, then obviously it should be removed.

@ywk253100
Copy link
Contributor Author

@ywk253100 I think these changes look great! I am not familiar with Azure permissions, so i'd also recommend getting a second review from another maintainer/community member to have a second pair of eyes on the tech pieces of this PR.

I'm approving the changes, but have one question/suggestion. Is the information in the removed Storage account, snapshot and disk management sections still valid? I like the way the required permissions are listed out with bullet points. I think it makes the information very clear to read and scan for. If it's still correct, i'd suggest keep it in the doc right before the improved section on how to create roles and assign these permissions that you added.

You have included the information within the new content you added, so it's maybe not necessary, but it might be more clear to have it both ways for users. If it's no longer valid, then obviously it should be removed.

It is still valid, I will take it back as you suggested, thanks.

@ywk253100 ywk253100 force-pushed the 220406_permission_doc branch 2 times, most recently from 6048ea1 to 5e5cde3 Compare April 15, 2022 01:30
Refine the minimum permissions needed by Velero in README

Signed-off-by: Wenkai Yin(尹文开) <[email protected]>
@ywk253100 ywk253100 force-pushed the 220406_permission_doc branch from 5e5cde3 to 9cdb37d Compare April 15, 2022 01:31
@ywk253100 ywk253100 requested a review from blackpiglet April 15, 2022 01:35
Copy link

@a-mccarthy a-mccarthy left a comment

Choose a reason for hiding this comment

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

lgtm!

@blackpiglet blackpiglet merged commit 7620571 into vmware-tanzu:main Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants