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

ravening · 2019-12-03T15:08:34Z

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

wido

LGTM on the code

yadvr · 2020-01-28T05:31:20Z

@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)

yadvr

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 · 2020-01-30T09:56:27Z

@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

yadvr · 2020-01-31T10:04:10Z

thanks @ravening
@blueorangutan package

blueorangutan · 2020-01-31T10:04:29Z

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

blueorangutan · 2020-01-31T11:00:43Z

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

yadvr · 2020-01-31T11:04:30Z

@blueorangutan test

blueorangutan · 2020-01-31T11:05:31Z

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

yadvr · 2020-02-06T06:04:22Z

@blueorangutan package

blueorangutan · 2020-02-06T06:04:55Z

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

yadvr · 2020-02-06T06:05:57Z

I did not test it.

blueorangutan · 2020-02-06T06:44:53Z

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

DaanHoogland

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)

DaanHoogland · 2020-02-06T16:24:48Z

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


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

DaanHoogland · 2020-02-06T16:26:59Z

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


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

DaanHoogland · 2020-02-06T16:30:50Z

+ 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);


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

DaanHoogland · 2020-02-06T17:08:07Z

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.

andrijapanicsb · 2020-02-07T12:18:23Z

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

yadvr · 2020-02-12T08:05:15Z

@blueorangutan package

blueorangutan · 2020-02-12T08:06:13Z

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

blueorangutan · 2020-02-12T10:09:00Z

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

GabrielBrascher · 2020-02-12T14:17:01Z

@blueorangutan package

blueorangutan · 2020-02-12T14:17:11Z

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

blueorangutan · 2020-02-12T14:39:55Z

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

yadvr · 2020-02-14T06:33:12Z

@blueorangutan package

blueorangutan · 2020-02-14T06:33:17Z

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

blueorangutan · 2020-02-14T07:26:36Z

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

andrijapanicsb · 2020-02-14T15:56:18Z

will be regression tested in Trillian, when possible. thx

yadvr · 2020-02-17T15:38:23Z

@blueorangutan package

blueorangutan · 2020-02-17T15:38:33Z

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

blueorangutan · 2020-02-17T16:35:59Z

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

yadvr · 2020-02-18T04:22:18Z

@blueorangutan package

blueorangutan · 2020-02-18T04:22:35Z

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

blueorangutan · 2020-02-18T05:05:10Z

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

yadvr · 2020-02-18T08:41:27Z

@blueorangutan test

blueorangutan · 2020-02-18T08:41:36Z

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

blueorangutan · 2020-02-18T16:51:31Z

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

yadvr · 2020-02-19T07:39:28Z

@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 threadserver/src/main/java/com/cloud/network/security/SecurityGroupManagerImpl.java Outdated

ravening force-pushed the feature_update_securitygroup branch from 0d7ff87 to b1a5aa6Compare 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 d8a442eCompare 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:masterFeb 19, 2020

yadvr mentioned this pull request Jun 8, 2020
[BUG] Edit SG button in SG detail view apache/cloudstack-primate#404
Closed

Uh oh!

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!

DaanHooglandFeb 6, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHooglandFeb 6, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHooglandFeb 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

Uh oh!

blueorangutan commented Feb 17, 2020

Uh oh!