Add new command to update security group name by ravening · Pull Request #3739 · apache/cloudstack · GitHub

Description

By default, once we create a security group we cant change its name.
In this feature, we introduce a new API command "updateSecurityGroup"
which allows us to rename the security group name. Although we can't
change the name of the "default" security group.

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)

Screenshots (if appropriate):

Navigate to "Network" section in the main window and select "Security groups" from the drop-down menu in the top left section

Click on "Add Security group" to create a new security group

Once the security group is created you can rename it using the edit button

How Has This Been Tested?

Navigate to "Network" tab in the main UI.
Select "Security groups" from the drop-down menu in the top left.
Click on "Add Security group" on the top right to create a new security group.
Give any suitable name and create it.
Now select the security group you just created and click on edit button to enter the new name
This will rename the security group to the new name
Now again try to edit the security group name and enter blank spaces. You will get error message which indicates that security group name cannot be empty.

Testing

New integration test case has been added which validates different test cases in updating the security group name. This can be run using

nosetests --with-marvin --marvin-config=<config file> test/integration/component/test_update_security_group.py

LGTM on the code

@ravening few points:

Please follow and implement based more modern API implementation such as https://github.com/apache/cloudstack/blob/master/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java#L34 (put version, use APINAME etc.)
Move the test to smoke instead of component if it's a fast enough test
Add the new marvin test to .travis.yml (sort alphabetically)

Please see my comment.

By default, once we create a security group we cant change its name. In this feature, we introduce a new API command "updateSecurityGroup" which allows us to rename the security group name. Although we can't change the name of the "default" security group.

@ravening few points:

Please follow and implement based more modern API implementation such as https://github.com/apache/cloudstack/blob/master/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java#L34 (put version, use APINAME etc.)

Move the test to smoke instead of component if it's a fast enough test

Add the new marvin test to .travis.yml (sort alphabetically)

@rhtyd I have made the necessary changes. Please review it again

thanks @ravening
@blueorangutan package

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-725

@blueorangutan test

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan package

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

I did not test it.

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-763

code lgtm, remarks are definitely not -1-worthy. I am planning a quick test in our lab (time and lab space are at short hand atm)

I'd rather see it constructed form the API name .toLower()+"response". no biggy

isn't this an exceptional situation? wouldn't there always be at least root admin responsible for creating a sec-group?

the message here could better be "unable to acquire lock for security group with id " + groupId

tested and works, just one remark. I think this is a generic details page problem in cloudstack but after saving, the old name is still used instead of the altered one.

tested and works, just one remark. I think this is a generic details page problem in cloudstack but after saving, the old name is still used instead of the altered one.

log out and in, or force refresh page - I recall similar issues on other parts of the gui

@blueorangutan package

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✖centos6 ✖centos7 ✔debian. JID-819

@blueorangutan package

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-841

@blueorangutan package

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-862

will be regression tested in Trillian, when possible. thx

@blueorangutan package

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✖centos6 ✔centos7 ✖debian. JID-884

@blueorangutan package

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-889

@blueorangutan test

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

Trillian test result (tid-1027)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28042 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3739-t1027-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 79 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	Failure	172.91	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	Failure	170.93	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	Failure	232.08	test_privategw_acl.py

@ravening can you enable the specific UI action in primate: https://github.com/apache/cloudstack-primate/blob/master/src/config/section/network.js#L188

By default, once we create a security group we cant change its name. In this feature, we introduce a new API command "updateSecurityGroup" which allows us to rename the security group name. Although we can't change the name of the "default" security group.

svenvogel added component:security-group type:enhancement labels Dec 4, 2019

wido reviewed Jan 15, 2020

View reviewed changes

Comment thread server/src/main/java/com/cloud/network/security/SecurityGroupManagerImpl.java Resolved

ravening force-pushed the feature_update_securitygroup branch from 0d7ff87 to b1a5aa6 Compare January 16, 2020 09:12

wido self-requested a review January 16, 2020 09:20

wido approved these changes Jan 16, 2020

View reviewed changes

yadvr added this to the 4.14.0.0 milestone Jan 28, 2020

yadvr requested changes Jan 29, 2020

View reviewed changes

ravening force-pushed the feature_update_securitygroup branch from b1a5aa6 to d8a442e Compare January 30, 2020 09:55

yadvr approved these changes Feb 6, 2020

View reviewed changes

yadvr requested a review from DaanHoogland February 6, 2020 06:05

DaanHoogland approved these changes Feb 6, 2020

View reviewed changes

apache deleted a comment from blueorangutan Feb 13, 2020

yadvr merged commit 4ab6b42 into apache:master Feb 19, 2020

yadvr mentioned this pull request Jun 8, 2020

[BUG] Edit SG button in SG detail view apache/cloudstack-primate#404

Closed

+                          public SecurityGroupVO doInTransaction(TransactionStatus status) {
+                              SecurityGroupVO group = _securityGroupDao.lockRow(groupId, true);
+                              if (group == null) {
+                                  throw new InvalidParameterValueException("Unable to find security group by id " + groupId);

+                  @Override
+                  public String getCommandName() {
+                      return s_name;

+                          return securityGroup.getAccountId();
+                      }
+                      return Account.ACCOUNT_ID_SYSTEM; // no account info given, parent this command to SYSTEM so ERROR events are tracked

Add new command to update security group name by ravening · Pull Request #3739 · apache/cloudstack · GitHub

Repository navigation

Add new command to update security group name - #3739

Add new command to update security group name#3739yadvr merged 1 commit intoapache:masterapache/cloudstack:masterfrom ravening:feature_update_securitygroupravening/cloudstack:feature_update_securitygroupCopy head branch name to clipboard

Conversation

ravening commented Dec 3, 2019

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Testing

Uh oh!

Uh oh!

wido left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jan 28, 2020

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

ravening commented Jan 30, 2020

Uh oh!

yadvr commented Jan 31, 2020

Uh oh!

blueorangutan commented Jan 31, 2020

Uh oh!

blueorangutan commented Jan 31, 2020

Uh oh!

yadvr commented Jan 31, 2020

Uh oh!

blueorangutan commented Jan 31, 2020

Uh oh!

yadvr commented Feb 6, 2020

Uh oh!

blueorangutan commented Feb 6, 2020

Uh oh!

yadvr commented Feb 6, 2020

Uh oh!

blueorangutan commented Feb 6, 2020

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Feb 6, 2020

Uh oh!

andrijapanicsb commented Feb 7, 2020

Uh oh!

yadvr commented Feb 12, 2020

Uh oh!

blueorangutan commented Feb 12, 2020

Uh oh!

blueorangutan commented Feb 12, 2020

Uh oh!

GabrielBrascher commented Feb 12, 2020

Uh oh!

blueorangutan commented Feb 12, 2020

Uh oh!

blueorangutan commented Feb 12, 2020

Uh oh!

yadvr commented Feb 14, 2020

Uh oh!

blueorangutan commented Feb 14, 2020

Uh oh!

blueorangutan commented Feb 14, 2020

Uh oh!

andrijapanicsb commented Feb 14, 2020

Uh oh!

yadvr commented Feb 17, 2020

Uh oh!

blueorangutan commented Feb 17, 2020

Add new command to update security group name#3739
yadvr merged 1 commit into
apache:masterfrom
ravening:feature_update_securitygroup