CLOUDSTACK-10294: PEP-8 fixes and enhancements to security_group.py by wido · Pull Request #2432 · apache/cloudstack

wido · 2018-01-26T10:16:33Z

The code in security_group.py is very old.

I looked into using Libvirt network filters for this, but it's lacking functionality we require.

This is my first attempt in cleaning up this file. There is a lot more work to do.

I tested this on a local system with Basic Networking and that worked just fine in my case.

The quality of the code is still horrible though:

Your code has been rated at 3.60/10 (previous run: 3.56/10, +0.04)

A lot more work is needed, but I'm taking baby-steps so that it makes the review a bit easier and not one big bang with changes.

We should return a boolean and not a String 'true' or 'false' Although this output is never checked by the calling function(s). Signed-off-by: Wido den Hollander <wido@widodh.nl>

Do not use == False or == None as that is not according to the Python specs. Signed-off-by: Wido den Hollander <wido@widodh.nl>

Calling just print 'hello' is deprecated and won't work in neweer Python versions. We should use the print() function Signed-off-by: Wido den Hollander <wido@widodh.nl>

Signed-off-by: Wido den Hollander <wido@widodh.nl>

This is the Python way of executing commands Signed-off-by: Wido den Hollander <wido@widodh.nl>

borisstoyanov

LGTM based on code review

DaanHoogland · 2018-01-26T10:53:05Z

#BabySteps save the world LGTM

yadvr

LGTM, subject to testing.
@wido you may include this script to run against pep8 during Travis's job0. See tools/travis for other such checks.

yadvr · 2018-02-10T17:30:54Z

@blueorangutan package

blueorangutan · 2018-02-10T17:31:47Z

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

blueorangutan · 2018-02-10T18:53:11Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1703

wido · 2018-02-14T14:07:00Z

We can merge this one, right?

yadvr · 2018-02-14T16:10:55Z

@wido let me kick a test round.
@blueorangutan test

blueorangutan · 2018-02-14T16:11:04Z

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

blueorangutan · 2018-02-14T22:07:23Z

Trillian test result (tid-2259)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 19611 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2432-t2259-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 66 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_hostha_enable_ha_when_host_in_maintenance	`Error`	1.07	test_hostha_kvm.py

borisstoyanov

Tests are looking good

yadvr · 2018-02-15T09:38:52Z

@wido can you add a JIRA id for this?

wido · 2018-02-15T13:11:34Z

Done @rhtyd , updated the title with the issue: https://issues.apache.org/jira/browse/CLOUDSTACK-10294

In 6233a77 as a part of PR apache#2432 the bash() function was replaced by the execute() function. Somehow this last calling of the bash() function was not caught by testing and is still in there. This causes Exceptions to be thrown by the Security Group script. Signed-off-by: Wido den Hollander <wido@widodh.nl>

…#2732) In 6233a77 as a part of PR #2432 the bash() function was replaced by the execute() function. Somehow this last calling of the bash() function was not caught by testing and is still in there. This causes Exceptions to be thrown by the Security Group script. Signed-off-by: Wido den Hollander <wido@widodh.nl>

…apache#2732) In 6233a77 as a part of PR apache#2432 the bash() function was replaced by the execute() function. Somehow this last calling of the bash() function was not caught by testing and is still in there. This causes Exceptions to be thrown by the Security Group script. Signed-off-by: Wido den Hollander <wido@widodh.nl>

blueorangutan · 2019-02-23T20:44:11Z

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

wido added 8 commits January 26, 2018 10:47

security_group.py: Use true Booleans instead of Strings
077013e
We should return a boolean and not a String 'true' or 'false' Although this output is never checked by the calling function(s). Signed-off-by: Wido den Hollander <wido@widodh.nl>

security_group.py: Use if (not) for checking variables
a172f54
Do not use == False or == None as that is not according to the Python specs. Signed-off-by: Wido den Hollander <wido@widodh.nl>

security_group.py: Use print() function instead of print
6f446d4
Calling just print 'hello' is deprecated and won't work in neweer Python versions. We should use the print() function Signed-off-by: Wido den Hollander <wido@widodh.nl>

security_group.py: Remove unused and commented function
c7c86c5
Signed-off-by: Wido den Hollander <wido@widodh.nl>

security_group.py: Use logging.warning() instead of logging.warn()
99f34a1
Signed-off-by: Wido den Hollander <wido@widodh.nl>

security_group.py: Add and remove blank lines for PEP-8
d18d2dc
Signed-off-by: Wido den Hollander <wido@widodh.nl>

security_group.py: Remove trailing ; as this is not needed in Python
127e9be
Signed-off-by: Wido den Hollander <wido@widodh.nl>

security_group.py: Use subprocess.check_output() for execution
f87aa87
This is the Python way of executing commands Signed-off-by: Wido den Hollander <wido@widodh.nl>

wido added component:kvm component:security-group labels Jan 26, 2018

wido requested review from GabrielBrascher, marcaurele and yadvr January 26, 2018 10:16

borisstoyanov approved these changes Jan 26, 2018
View reviewed changes

yadvr approved these changes Feb 10, 2018
View reviewed changes

borisstoyanov approved these changes Feb 15, 2018
View reviewed changes

wido changed the title ~~KVM: PEP-8 fixes and enhancements to security_group.py~~CLOUDSTACK-10294: PEP-8 fixes and enhancements to security_group.pyFeb 15, 2018

yadvr merged commit 6233a77 into apache:masterFeb 15, 2018

yadvr added this to the 4.12.0.0 milestone May 3, 2018

wido mentioned this pull request Jul 5, 2018
security_group: Use execute() function instead of non-existing bash() #2732
Merged

Uh oh!

Conversation

wido commented Jan 26, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jan 26, 2018

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Feb 10, 2018

Uh oh!

blueorangutan commented Feb 10, 2018

Uh oh!

blueorangutan commented Feb 10, 2018

Uh oh!

wido commented Feb 14, 2018

Uh oh!

yadvr commented Feb 14, 2018

Uh oh!

blueorangutan commented Feb 14, 2018

Uh oh!

blueorangutan commented Feb 14, 2018

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Feb 15, 2018

Uh oh!

wido commented Feb 15, 2018

Uh oh!

blueorangutan commented Feb 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wido commented Jan 26, 2018•
edited
Loading