FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Allow users to update volume name by ravening · Pull Request #4584 · apache/cloudstack · GitHub

  1. apache
  2. cloudstack

Repository navigation

Allow users to update volume name - #4584

Closed
ravening wants to merge 0 commit into
apache:masterfrom
ravening:master
ConversationCommits (0)ChecksFiles changed
Closed

Allow users to update volume name#4584
ravening wants to merge 0 commit into
apache:masterfrom
ravening:master

Conversation

ravening commented Jan 14, 2021

Copy link
Copy Markdown
Member

Description

Provide an api support to update volume name by all users
Can be done both through api and UI

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Through API

update volume id=3e8f9553-e1c9-4ac0-bb43-10d7d928daf2 name=test-admin

ravening mentioned this pull request Jan 14, 2021
Open
RodrigoDLopez suggested changes Jan 15, 2021
View reviewed changes

RodrigoDLopez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I am trying to understand in which scenario it is necessary to give freedom for ordinary users to change names within the environment.
You can explain it to me because I couldn't reach a conclusion on my own.

Comment thread api/src/main/java/org/apache/cloudstack/api/command/user/volume/UpdateVolumeCmd.java Outdated
type = CommandType.STRING,
description = "The chain info of the volume",
since = "4.4")
since = "4.4", authorized = {RoleType.Admin})

RodrigoDLopez Jan 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Correct me if I'm wrong, but I believe that only administrators can execute the updateVolume command.

ravening Jan 19, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

admins and regular users can run this command. but regular users can only change name

Comment thread api/src/main/java/org/apache/cloudstack/api/command/user/volume/UpdateVolumeCmd.java Outdated
description = "an optional field, whether to the display the volume to the end user or not.", authorized = {RoleType.Admin})
private Boolean displayVolume;

@Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "new name of the volume", since = "4.16")

RodrigoDLopez Jan 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You put roleAdmin in all the other parameters of this API, but not this one.
Any reason for this behavior? users without administrator permission will not be able to run this command. And ordinary users shouldn't be able to change the name of what doesn't belong to them.

It would be nice to capitalize the n in new as well

ravening Jan 19, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Other parameters have sensitive info whereas the "name" parameter is not critical.
So regular users can change only name but not other important values

Comment thread server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java Outdated
Account caller = CallContext.current().getCallingAccount();
if (!_accountMgr.isRootAdmin(caller.getId())) {
if (path != null || state != null || storageId != null || displayVolume != null || customId != null || chainInfo != null) {
throw new InvalidParameterValueException("The domain admin and normal user are not allowed to update volume except volume name");

RodrigoDLopez Jan 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

does it make sense to allow ordinary users the power to change names within the environment?

yadvr added this to the 4.16.0.0 milestone Jan 18, 2021

ravening commented Jan 19, 2021

Copy link
Copy Markdown
Member Author

I am trying to understand in which scenario it is necessary to give freedom for ordinary users to change names within the environment.
You can explain it to me because I couldn't reach a conclusion on my own.

@RodrigoDLopez there is no harm in changing the volume name. so anybody can do it

RodrigoDLopez approved these changes Jan 19, 2021
View reviewed changes

RodrigoDLopez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@ravening I know that changing volume names does no harm, but I don't like empowering users even if they are cosmetic powers.
But you have my approval and my +1

weizhouapache commented Jan 19, 2021

Copy link
Copy Markdown
Member

code lgtm

ravening closed this Jan 26, 2021

mib1185 commented Jan 26, 2021

Copy link
Copy Markdown
Contributor

Hi @ravening
why did you close this PR before merge? 🤔

ravening commented Jan 26, 2021

Copy link
Copy Markdown
Member Author

Hi @ravening
why did you close this PR before merge? 🤔

@mib1185 sorry it got closed after some rebase with my branch.. I have opened new pr with ui changes in #4618

DaanHoogland mentioned this pull request Jan 27, 2021
Merged
3 of 12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

DaanHoogland DaanHoogland approved these changes
weizhouapache weizhouapache approved these changes
+1 more reviewer
RodrigoDLopez RodrigoDLopez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

complexity:simple a few days to a couple of weeks Severity:Minor type:enhancement

Projects

None yet

Milestone

4.16.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL