Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

CLOUDSTACK-10294: PEP-8 fixes and enhancements to security_group.py#2432

Merged
yadvr merged 8 commits intoapache:masterapache/cloudstack:masterfrom
wido:security_group-pep8wido/cloudstack:security_group-pep8Copy head branch name to clipboard
Feb 15, 2018
Merged

CLOUDSTACK-10294: PEP-8 fixes and enhancements to security_group.py#2432
yadvr merged 8 commits intoapache:masterapache/cloudstack:masterfrom
wido:security_group-pep8wido/cloudstack:security_group-pep8Copy head branch name to clipboard

Conversation

@wido
Copy link
Contributor

@wido wido commented Jan 26, 2018 •
edited
Loading

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.

wido added 8 commits January 26, 2018 10:47
@wido
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>
@wido
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>
@wido
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>
@wido
security_group.py: Remove unused and commented function
c7c86c5 
Signed-off-by: Wido den Hollander <wido@widodh.nl>
@wido
security_group.py: Use logging.warning() instead of logging.warn()
99f34a1 
Signed-off-by: Wido den Hollander <wido@widodh.nl>
@wido
security_group.py: Add and remove blank lines for PEP-8
d18d2dc 
Signed-off-by: Wido den Hollander <wido@widodh.nl>
@wido
security_group.py: Remove trailing ; as this is not needed in Python
127e9be 
Signed-off-by: Wido den Hollander <wido@widodh.nl>
@wido
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>
borisstoyanov
borisstoyanov approved these changes Jan 26, 2018
View reviewed changes
Copy link
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

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

LGTM based on code review

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Jan 26, 2018

#BabySteps save the world LGTM

yadvr
yadvr approved these changes Feb 10, 2018
View reviewed changes
Copy link
Member

@yadvr yadvr left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

yadvr commented Feb 10, 2018

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Feb 10, 2018

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

@blueorangutan
Copy link

blueorangutan commented Feb 10, 2018

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

@wido
Copy link
Contributor Author

wido commented Feb 14, 2018

We can merge this one, right?

@yadvr
Copy link
Member

yadvr commented Feb 14, 2018

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

@blueorangutan
Copy link

blueorangutan commented Feb 14, 2018

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

@blueorangutan
Copy link

blueorangutan commented Feb 14, 2018

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
borisstoyanov approved these changes Feb 15, 2018
View reviewed changes
Copy link
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

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

Tests are looking good

@yadvr
Copy link
Member

yadvr commented Feb 15, 2018

@wido can you add a JIRA id for this?

@wido wido changed the title KVM: PEP-8 fixes and enhancements to security_group.py CLOUDSTACK-10294: PEP-8 fixes and enhancements to security_group.py Feb 15, 2018
@wido
Copy link
Contributor Author

wido commented Feb 15, 2018

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

@yadvr yadvr merged commit 6233a77 into apache:master Feb 15, 2018
@yadvr yadvr added this to the 4.12.0.0 milestone May 3, 2018
wido added a commit to wido/cloudstack that referenced this pull request Jul 5, 2018
@wido
security_group: Use execute() function instead of non-existing bash()
d99d68c 
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>
@wido wido mentioned this pull request Jul 5, 2018
Merged
wido added a commit to wido/cloudstack that referenced this pull request Jul 18, 2018
@wido
security_group: Use execute() function instead of non-existing bash()
9c2fa03 
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>
DaanHoogland pushed a commit that referenced this pull request Jul 20, 2018
@wido @DaanHoogland
security_group: Use execute() function instead of non-existing bash() (…
1ccb32f 
…#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>
borisstoyanov pushed a commit to shapeblue/cloudstack that referenced this pull request Jul 23, 2018
@wido @borisstoyanov
security_group: Use execute() function instead of non-existing bash() (…
761e94a 
…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
Copy link

blueorangutan commented Feb 23, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes

@borisstoyanov borisstoyanov borisstoyanov approved these changes

@marcaurele marcaurele Awaiting requested review from marcaurele

@GabrielBrascher GabrielBrascher Awaiting requested review from GabrielBrascher

Assignees

No one assigned

Labels

component:kvm component:security-group

Projects

None yet

Milestone

4.12.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@wido @DaanHoogland @yadvr @blueorangutan @borisstoyanov
Morty Proxy This is a proxified and sanitized view of the page, visit original site.