Fix Python code checkstyle execute by "systemvm\test\runtests.sh"#2576
Conversation
|
@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1935 |
rafaelweingartner
force-pushed
the
fixPythonCodeStyle
branch
13 times, most recently
from
f4076c3 to
371cac5
Compare
|
@DaanHoogland, @rhtyd, @wido and others Python gurus. Can you guys take a look at this PR? I am not a Python savvy, so I cannot explain why I needed to apply the changes I did. I basically googled the problems and applied the suggested solutions. Among the things I needed to code, there are changes related to indentations and one extra line that the “pep8” wanted. |
wido
left a comment
wido
left a comment
There was a problem hiding this comment.
LGTM to me. Just cosmetic changes to the Python files
|
@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1936 |
| pep8 --max-line-length=179 *py | ||
| pep8 --max-line-length=179 --exclude=monitorServices.py,baremetal-vr.py,passwd_server_ip.py `find ../debian -name \*.py` | ||
| echo "Running pycodestyle to check systemvm/python code for errors" | ||
| pycodestyle --max-line-length=179 *py |
There was a problem hiding this comment.
I was getting a message that pep8 is deprecated and is being replaced by pycodestyle, that is why I replaced. It was one of my commits trying to solve the problem (or at least change some error message).
tools/travis/before_install.sh
Outdated
| echo -e "\nInstalling some python packages: " | ||
|
|
||
| pip install --user --upgrade pip | ||
| pip uninstall pylint |
There was a problem hiding this comment.
@rafaelweingartner Why uninstall here? I see pylint is installed again by pip at line 106.
There was a problem hiding this comment.
¯_(ツ)_/¯...
I really do not understand this. Most of the results I found on Google were telling me to remove and install again the library. I guess when people do not know what they are doing, they do this things.
Well, I will remove and see what happens.
There was a problem hiding this comment.
Thanks, np I was just curious why we were doing it that way. (love the expression of the smiley btw)
tools/travis/before_install.sh
Outdated
| for ((i=0;i<$RETRY_COUNT;i++)) | ||
| do | ||
| pip install --user --upgrade lxml paramiko nose texttable ipmisim pyopenssl mock flask netaddr pylint pep8 > /tmp/piplog | ||
| pip install --user --upgrade lxml paramiko texttable ipmisim pyopenssl mock flask netaddr nose pylint pycodestyle six astroid Markdown > /tmp/piplog |
There was a problem hiding this comment.
@rafaelweingartner are we using new pkgs - six astroid Markdown, where/how?
There was a problem hiding this comment.
Markdown I added because I was getting a message saying that it is not installed, but some of these dependencies might require it. That is why I installed.
six and astroid seems to be used by pylint and nose. And I think that is the problem we were having. There is an issue regarding these dependencies versions, which are not automatically upgraded when you install some library the relies on them.
¯\_(ツ)_/¯
There was a problem hiding this comment.
@rafaelweingartner ideally, pip install should install additional dependencies we should not need to install dependencies for pylint manually.
There was a problem hiding this comment.
Another note, the new release of pip (v10) might deprecate few things -- I know python 2.6.x is not well supported now.
There was a problem hiding this comment.
I think the problems happens when the dependencies are already installed. Then, it seems that for some cases pip does not upgrade them if you do not ask to.
Again, I am not a Python guy, so that that hypothesis with a grain of salt.
There was a problem hiding this comment.
If dependencies are already installed, it should not cause an issue. The --upgrade should upgrade an installed package and its dependencies, but nonethless let's see how the Travis run goes.
There was a problem hiding this comment.
That is what I expected to happen, but it seems it did not work.
|
Thanks @rafaelweingartner I see the issue is Travis related, thanks for fixing. I've left some remarks. Also, the Travis failure affects 4.11 branch too, can you rebase and edit this PR against 4.11? On latest master: |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
@rhtyd Locally my tests were also passing :(... Actually, locally I received those problems regarding code formatting. That is why I fixed them too. However, even after fixing all of them, Travis would not work. Then, I started digging deeper in the dependencies and looking for incompatibilities (this is a nightmare!). |
yadvr
left a comment
yadvr
left a comment
There was a problem hiding this comment.
LGTM (some remarks left, but none are blockers). Please however re-target the PR for 4.11 as the same issues fail 4.11 Travis jobs.
|
I'll kick test once the PR is re-targeted for 4.11 branch @rafaelweingartner |
rafaelweingartner
force-pushed
the
fixPythonCodeStyle
branch
from
371cac5 to
b9dae29
Compare
|
@rhtyd rebased against 4.11. I also added only the dependencies upgrade. Let's see what happens. |
|
Well, now, the error in Travis are related to the code style. I will apply the code style fixes too then. |
|
Nevermind, @rafaelweingartner please kick packaging job with BO once you're able to re-fix the styles issues. Sorry for the additional trouble on porting to 4.11. |
|
@blueorangutan package. @rhtyd I think everything is fine now. |
|
@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1937 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-2515)
|
|
@rafaelweingartner a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1942 |
|
Trillian test result (tid-2517)
|
Tipo selecionado no _update traffic type_ usa como base o escolhido na aba _Traffic types_ Closes apache#2576 See merge request scclouds/scclouds!1172
5 participants
Description
I noticed a failure in travis for PR #2574 relating to Python code check style. While investigating that error, I found out that when running
cloudstack/systemvm/test/runtests.shI was receiving the following errors:That command is executed by Travis. After fixing these issues. The "systemvm\test\runtests.sh" stopped failing.
Types of changes
How Has This Been Tested?
Locally
Checklist:
Testing
@blueorangutan package