Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Python interpreter discovery #50163

Merged
merged 3 commits into from
Feb 28, 2019
Merged

Python interpreter discovery #50163

merged 3 commits into from
Feb 28, 2019

Conversation

nitzmahone
Copy link
Member

SUMMARY
  • No longer blindly default to only /usr/bin/python
  • ansible_python_interpreter defaults to auto_legacy, which will discover the platform Python interpreter on some platforms (but still favor /usr/bin/python if present for backward compatibility). Use auto to always use the discovered interpreter.
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

BaseAction

ADDITIONAL INFORMATION

@ansibot
Copy link
Contributor

ansibot commented Dec 19, 2018

cc @acozine
click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.8 This issue/PR affects Ansible v2.8 docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Dec 19, 2018
@abadger
Copy link
Contributor

abadger commented Dec 19, 2018

Suggestion: Since all of the bugs I found in the script that's shipped over to the remote host involve what happens to os-release, it might be better to simply read and return os-release to the controller. We can then use libraries on the controller to parse it and decide what to do.

If we always return os-release and the results of platform.dist if available, that could also give us the opportunity to override platform.dist if we need to.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Dec 19, 2018
abadger
abadger reviewed Dec 19, 2018
View reviewed changes
lib/ansible/plugins/action/__init__.py Outdated
# FIXME: use one invocation of `command -v` with multiple values and a multiline regex instead?
exec_tmpl = "echo FOUND `command -v '{0}'` ENDFOUND"

python_locator = self._connection._shell._SHELL_AND.join(["uname", ";".join([exec_tmpl.format(t) for t in bootstrap_python_list])])
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be done in the shell plugin

@ansibot

This comment has been minimized.

Sign in to view
abadger
abadger reviewed Dec 19, 2018
View reviewed changes
lib/ansible/config/base.yml Outdated
rhel: *rhelish
ubuntu:
'14': /usr/bin/python
'16': /usr/bin/python3
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way for users to specify dicts in their config files? Or is this a config setting that they won't be able to override because they can't match the data format?

Copy link
Member

Choose a reason for hiding this comment

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

There is no ini or env here, so it wouldn't be modifiable via config. Also, until we support YAML configs, it wouldn't be settable by a user even if it was exposed.

abadger
abadger reviewed Dec 19, 2018
View reviewed changes
lib/ansible/executor/module_common.py Outdated
return self.message

def __repr__(self):
return self.message
Copy link
Contributor

Choose a reason for hiding this comment

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

repr should say how to recreate the class whenever possible:

>>> repr(err)
"ZeroDivisionError('integer division or modulo by zero',)"
>>> print(err)
integer division or modulo by zero
Suggested change
return self.message
return 'InterpreterDiscoveryRequiredError({0}, {1}, {2})'.format(self.message, self.interpreter_name, self.discover_mode)

sivel
sivel reviewed Dec 19, 2018
View reviewed changes
lib/ansible/config/base.yml Outdated Show resolved Hide resolved
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 27, 2018
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 17, 2019
@nitzmahone nitzmahone force-pushed the auto_python_interpreter branch 2 times, most recently from 66c5d11 to 283c627 Compare January 22, 2019 01:30
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 22, 2019
@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Feb 13, 2019
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Feb 13, 2019
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Feb 19, 2019
acozine
acozine reviewed Feb 22, 2019
View reviewed changes
Copy link
Contributor

@acozine acozine left a comment

Choose a reason for hiding this comment

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

Made suggested changes in a branch - see https://github.com/acozine/ansible/commits/PR50163.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 22, 2019
@ansibot ansibot removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Feb 22, 2019
nitzmahone and others added 2 commits February 27, 2019 13:32
@nitzmahone
Python interpreter discovery
a20bc87 
* No longer blindly default to only `/usr/bin/python`
* `ansible_python_interpreter` defaults to `auto_legacy`, which will discover the platform Python interpreter on some platforms (but still favor `/usr/bin/python` if present for backward compatibility). Use `auto` to always use the discovered interpreter, append `_silent` to either value to suppress warnings.
* includes new doc utility method `get_versioned_doclink` to generate a major.minor versioned doclink against docs.ansible.com (or some other config-overridden URL)
@acozine @nitzmahone
docs revisions for python interpreter discovery
2458544 
(cherry picked from commit 5b53c0012ab7212304c28fdd24cb33fd8ff755c2)
@nitzmahone nitzmahone changed the title [WIP] Python interpreter discovery Python interpreter discovery Feb 28, 2019
@nitzmahone
Copy link
Member Author

rebuild_merge

@ansibot ansibot added shipit This PR is ready to be merged by Core needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. shipit This PR is ready to be merged by Core labels Feb 28, 2019
@nitzmahone
Copy link
Member Author

rebuild_merge

@nitzmahone
Copy link
Member Author

Unstable tests appear to be Fedora-infra, unrelated... Merging by request.

@nitzmahone nitzmahone merged commit 4d3a612 into ansible:devel Feb 28, 2019
@karolistamutis karolistamutis mentioned this pull request Mar 19, 2019
Merged
This was referenced Mar 20, 2019
Closed
Closed
Closed
Closed
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sivel sivel sivel left review comments

@abadger abadger abadger left review comments

@maxamillion maxamillion maxamillion left review comments

@acozine acozine acozine left review comments

@hroncok hroncok hroncok left review comments

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. feature This issue/PR relates to a feature request. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@nitzmahone @ansibot @abadger @sivel @maxamillion @acozine @hroncok