-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not allow python2 to be added to salt-ssh tar by default #58389
Conversation
re-run full all |
re-run full all |
1 similar comment
re-run full all |
re-run full all |
re-run full all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
from tests.support.mock import MagicMock, patch | ||
|
||
|
||
@pytest.fixture() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pytest.fixture() | |
@pytest.fixture |
"argv": argv, | ||
"__role": "master", | ||
"cachedir": tmpdir.strpath, | ||
"extension_modules": tmpdir.join("extmods"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"extension_modules": tmpdir.join("extmods"), | |
"extension_modules": tmpdir.join("extmods").strpath, |
@s0undt3ch ready for re-review |
if _six.PY2: | ||
import concurrent | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this if _six.PY2
check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This keeps support for https://docs.saltstack.com/en/master/topics/ssh/ssh_ext_alternatives.html which allows you to bundle up an old version of salt that uses python 2 to use on the target host. This PR removes salt-ssh trying to automatically add python2 libraries automatically, but if a user is intentionally using ssh_ext_alternatives
I think they should still be able to add python 2 libraries for older versions of salt.
salt/utils/thin.py
Outdated
@@ -381,7 +389,7 @@ def _get_supported_py_config(tops, extended_cfg): | |||
for the supported Python interpreter versions. This is then written into the thin.tgz | |||
archive and then verified by salt.client.ssh.ssh_py_shim.get_executable() | |||
|
|||
Note: Minimum default of 2.x versions is 2.7 and 3.x is 3.0, unless specified in namespaces. | |||
Note: Minimum default of 3.x is 3.0, unless specified in namespaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure on 3.0, salt is only meant to work with 3.5, right?
Or for this piece of code it doesn't really matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will re-word this. You can even actually use 2.7 if using an older version of salt.
What does this PR do?
After dropping python 2 support from Salt we should not be adding python 2 modules to the salt-ssh binary by default. This PR addresses this by removing the code that automatically looked for python 2 modules and added them to the tar. This also ensures that
ssh_ext_alternatives
can still work with older python 2 supported salt versions. For example, when usingssh_ext_alternatives
you can use the 2019.2.3 version of salt that is copied over in the tar. This version still supports Python 2 so we should still allow a user to use this version when using thessh_ext_alternatives
feature.What issues does this PR fix or reference?
Fixes: #57647
Previous Behavior
Misleading error when python 3 is not installed on the target host:
and python 2 modules added to the tar even though they will not be used.
New Behavior
New Error to inform user they need to install Python 3 on target host.
Merge requirements satisfied?
Commits signed with GPG?
Yes