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.

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

Emit warning when running on the controller with a Python older than 3.8 #72467

Merged
merged 8 commits into from
Nov 9, 2020

Conversation

sivel
Copy link
Member

@sivel sivel commented Nov 3, 2020

SUMMARY

Emit warning when running on the controller with a Python older than 3.8

Ansible 2.11 will make Python 3.8 a soft dependency for the controller. Ansible 2.12 will require Python3.8 or newer to function on the controller.

This deprecation warning will help inform users of this change.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

changelogs/fragments/controller-python-warning.yml Outdated Show resolved Hide resolved
lib/ansible/cli/scripts/ansible_cli_stub.py Outdated Show resolved Hide resolved
lib/ansible/config/base.yml Outdated Show resolved Hide resolved
@ansibot ansibot added affects_2.11 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. python3 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. test This PR relates to tests. labels Nov 3, 2020
@samdoran
Copy link
Contributor

samdoran commented Nov 3, 2020

This should be mentioned in the docs as well: docs/docsite/rst/installation_guide/intro_installation.rst.

Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

Warning LGTM, though is this also the time/place to bump python_requires to 3.8?

@@ -0,0 +1,3 @@
minor_changes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be higher than minor so it's more prominent in the changelog?

Copy link
Member

Choose a reason for hiding this comment

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

The warning itself is minor

Copy link
Contributor

Choose a reason for hiding this comment

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

The policy change is major. The changelog & porting guides are reviewed by people before upgrading. Let's make sure we over-communicate these changes.

@sivel
Copy link
Member Author

sivel commented Nov 3, 2020

though is this also the time/place to bump python_requires to 3.8?

I'm concerned if we do that, fewer people will ever see the warning. But honestly, since we only plan to package DEB and RPM for 3.8+, I'd be fine with doing it in the python package too.

@sivel
Copy link
Member Author

sivel commented Nov 3, 2020

@samdoran docs entry added.

@ansibot ansibot added the docs This issue/PR relates to or includes documentation. label Nov 3, 2020
@ansibot
Copy link
Contributor

ansibot commented Nov 3, 2020

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/cli/scripts/ansible_cli_stub.py:77:12: ansible-deprecated-no-collection-name: No collection name found in call to Display.deprecated or AnsibleModule.deprecate

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Nov 3, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Nov 4, 2020
@webknjaz
Copy link
Member

webknjaz commented Nov 4, 2020

I'm concerned if we do that, fewer people will ever see the warning. But honestly, since we only plan to package DEB and RPM for 3.8+, I'd be fine with doing it in the python package too.

If we also add python_requires, close to 100% of the users won't ever see it. I don't know a single individual who would use pip's "ignore checks" args. Meaning that we could as well just go ahead and use py38 syntax.

@webknjaz
Copy link
Member

webknjaz commented Nov 4, 2020

Also, python_requires will only work if we'll actually ship wheels so that this metadata value would end up in the index and pip would actually see which versions are incompatible with the local env.

@sivel sivel added this to In Progress in ansible-core 2.11 Nov 4, 2020
@nitzmahone
Copy link
Member

nitzmahone commented Nov 4, 2020

@webknjaz the primary constraint on 3.8 for Ansible 2.11 is around split controller/remote testing in CI. We're assuming that won't be available until later in the 2.11 dev cycle, so rather than stopping all testing on everything < 3.8 until then, we just decided to do the "packaging/support only" requirement for 2.11.

@mattclay mattclay removed the needs_triage Needs a first human triage before being processed. label Nov 5, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 5, 2020
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 5, 2020
setup.py Outdated
@@ -364,7 +364,7 @@ def get_dynamic_setup_params():
license='GPLv3+',
# Ansible will also make use of a system copy of python-six and
# python-selectors2 if installed but use a Bundled copy if it's not.
python_requires='>=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*',
python_requires='>=3.8',
Copy link
Member Author

Choose a reason for hiding this comment

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

I've gone ahead and added this change now. I can remove it, we can keep it, we can change it later. I'll discuss this with the other architects and confirm our plan.

@pabelanger
Copy link
Contributor

Thank you, could we have a comment around testing? Just to confirm, would this mean testing for integration move to only py38?

@ansibot

This comment has been minimized.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Nov 6, 2020
sivel and others added 8 commits November 6, 2020 13:59
Co-authored-by: Matt Clay <matt@mystile.com>
Co-authored-by: Matt Clay <matt@mystile.com>
Co-authored-by: Matt Clay <matt@mystile.com>
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Nov 6, 2020
@sivel
Copy link
Member Author

sivel commented Nov 9, 2020

Thank you, could we have a comment around testing? Just to confirm, would this mean testing for integration move to only py38?

The big reason we are not making this a hard dep, by changing the code to require Python 3.8, is due to testing. What is on the roadmap for the 2.11 release is to implement split controller testing, where we can have different Python deps for the controller from that of the target.

Right now in our CI, the controller and target are the same host. Once ansible-test has this functionality we'll drop support for Python versions older than 3.8.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 9, 2020
@sivel
Copy link
Member Author

sivel commented Nov 9, 2020

I'm merging this as is, without a python_requires change. If we decide to do that, we can do it later.

@sivel sivel merged commit 96ad5b7 into ansible:devel Nov 9, 2020
ansible-core 2.11 automation moved this from In Progress to Merged Nov 9, 2020
@ansible ansible locked and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.11 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. feature This issue/PR relates to a feature request. python3 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. test This PR relates to tests.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants