Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign uppatch st2-run-pack-tests to work with python3 #4956
Conversation
|
I see 2 places where this script is called: Adding a test of the -3 will require a git clone of the repo and setting the ST2_REPO_PATH and then running . |
|
@nzlosh Can you please help evaluating UX functionality/changes in this PR and review it? |
|
Can I have some more context about the problem this PR is addressing? On Ubuntu 18.04 the |
|
The centos7 install uses python2.7. Adding this switch will tell the pack
test script to NOT look for the st2 packages in the installed dir at
/opt/stackstorm.
Yes on ubuntu you do not need this. The ubunu install uses python3.
Furthermore, to test this correctly we need to run this script with the new
-3 flag on a centos7 box. You cant simply run tests on en empty box. And we
cant * that i know of * use the tox.ini because that wont have the versions
conflict with python.
To reproduce the bug this fixes try to run a python3 action script though
the original on Stackstorm centos7 installed box. You will see it try to
load up python2.7 libs into its pythonpath. And the test will fail.
In the end this is dead end code. But i have to use this version on our st2
installed testing box.
I submitted it to help others who run onto the same issue with testing on a
stackstorm centos7 box.
…On Sun, May 24, 2020, 05:14 Carlos ***@***.***> wrote:
Can I have some more context about the problem this PR is addressing? On
Ubuntu 18.04 the st2-run-pack-tests detects and uses python3.6 to test
the pack without any problem. Running the script with -3 returns must set
ST2_REPO_PATH variable
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4956 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACZ5TILXE57SDDDRAMQY25TRTDQQBANCNFSM4NGKB4LQ>
.
|
|
Essentially this flag says, "python is 2.7 and python3 is 3 on the system.
…On Mon, May 25, 2020, 11:32 aj jones ***@***.***> wrote:
The centos7 install uses python2.7. Adding this switch will tell the pack
test script to NOT look for the st2 packages in the installed dir at
/opt/stackstorm.
Yes on ubuntu you do not need this. The ubunu install uses python3.
Furthermore, to test this correctly we need to run this script with the
new -3 flag on a centos7 box. You cant simply run tests on en empty box.
And we cant * that i know of * use the tox.ini because that wont have the
versions conflict with python.
To reproduce the bug this fixes try to run a python3 action script though
the original on Stackstorm centos7 installed box. You will see it try to
load up python2.7 libs into its pythonpath. And the test will fail.
In the end this is dead end code. But i have to use this version on our
st2 installed testing box.
I submitted it to help others who run onto the same issue with testing on
a stackstorm centos7 box.
On Sun, May 24, 2020, 05:14 Carlos ***@***.***> wrote:
> Can I have some more context about the problem this PR is addressing? On
> Ubuntu 18.04 the st2-run-pack-tests detects and uses python3.6 to test
> the pack without any problem. Running the script with -3 returns must
> set ST2_REPO_PATH variable
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#4956 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACZ5TILXE57SDDDRAMQY25TRTDQQBANCNFSM4NGKB4LQ>
> .
>
|
|
Here is the complete Python 3:
Python 2:
|
|
@nzlosh did @guzzijones answer your question? |
|
Overall, adding support for Python3 looks good to me. There are a few points I've raised, such as allowing the users to choose the version of python used for the pack tests and some more explicit error messages. |
| @@ -78,10 +80,11 @@ function usage() { | |||
| echo " and virtualenv, if any, for subsequent test runs." >&2 | |||
| echo " -t : Run tests with timing enabled" >&2 | |||
| echo " -v : Verbose mode (log debug messages to stdout)." >&2 | |||
| echo " -3 : use python3" >&2 | |||
nzlosh
May 30, 2020
Contributor
Use a flag that takes an argument rather than hard coding 3 (hint: use $OPTARG to get the version).
3 doesn't give any meaning to the purpose of the flag, using P would be better so there is an association with P for Python. It would be useful to be able to specify a version rather than just the major version. E.g. accept 3.6, 3.7, 3.8 to give pack developers the flexibility to test against a specific minor version.
Remember to document the flag in the Usage: $0 section of the output too.
Use a flag that takes an argument rather than hard coding 3 (hint: use $OPTARG to get the version).
3 doesn't give any meaning to the purpose of the flag, using P would be better so there is an association with P for Python. It would be useful to be able to specify a version rather than just the major version. E.g. accept 3.6, 3.7, 3.8 to give pack developers the flexibility to test against a specific minor version.
Remember to document the flag in the Usage: $0 section of the output too.
| @@ -104,6 +107,14 @@ while getopts ":p:f:xjvct" o; do | |||
| t) | |||
| ENABLE_TIMING=true | |||
| ;; | |||
| 3) | |||
| PYTHON_3=true | |||
| if [ -z "$ST2_REPO_PATH" ]; then | |||
nzlosh
May 30, 2020
Contributor
Why does the python 3 flag short circuit the ST2_REPO_PATH logic which handles the case of its absence later in the script?
Why does the python 3 flag short circuit the ST2_REPO_PATH logic which handles the case of its absence later in the script?
guzzijones
May 30, 2020
Author
Contributor
This is the most important part. This script will look for st2 libs in \opt\stsckstorm if i dont set this. On centos7 those are python 2.7. Maybe what we do here is check the system python version vs. The version passed in this flag. If they doffer then force this env var to be set.
This is the most important part. This script will look for st2 libs in \opt\stsckstorm if i dont set this. On centos7 those are python 2.7. Maybe what we do here is check the system python version vs. The version passed in this flag. If they doffer then force this env var to be set.
| 3) | ||
| PYTHON_3=true | ||
| if [ -z "$ST2_REPO_PATH" ]; then | ||
| echo "must set ST2_REPO_PATH variable" |
nzlosh
May 30, 2020
Contributor
A clearer error message explaining what the ST2_REPO_PATH variable is used for will help people set the correct value. e.g. Environment variable ST2_REPO_PATH must be set to your st2 installation or st2 git repository for pack tests to function correctly.
A clearer error message explaining what the ST2_REPO_PATH variable is used for will help people set the correct value. e.g. Environment variable ST2_REPO_PATH must be set to your st2 installation or st2 git repository for pack tests to function correctly.
|
Just a heads up: |
|
@guzzijones Yes, we'll be good to accept this enhancement into st2 core, once it's polished. Thanks @nzlosh for helping with the review to get it closer for the broader use. |
|
@guzzijones any update on driving this PR to completion? |
allows st2-run-pack-tests to work with python3 on systems that have python3 installed as a separate binary.
I probably need to find and or create an integration test for this.