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

pip module: During --check, if latest is asked, check using pip list --outdated #72298

Closed
wants to merge 1 commit into from
Closed

Conversation

JulienPalard
Copy link
Contributor

Mabe fixes #62826

SUMMARY

In the pip module, I'm using pip list --outdated to list outdated Python packages, in order for --check to be able to tell if a package with state=latest is to be upgraded or not.

Current situation is to always return changed=True when a packaged is state=latest, even if it's at the latest version.

The diff may be bigger than absolutely needed:

  • During my "understand the code" phase, I noticed _is_present takes an unused parameter, so I dropped it, as it's really near my diffs anyway.
  • Again during the reading phase, I found the if module.check_mode a bit long for what it does (30 lines) itself in a 195 lines of code main function, I had hard times understanding it. So I took care of decreasing the complexity instead of increasing it during my implementation by moving a part dedicated to clean and enhance currently installed packaged in the function dedicated to it, and by implementing my code in a dedicated function too. Now the main is 188 lines (a tad shorter), and the if module.check_mode is 23 lines (with a feature added!).
  • I think there was a potential bug in the pkg_list = [p for p in out.split('\n')... line, using the out variable instead of the out_pip variable, the out variable could contain unexpected content from previous runs (setup venv, a few lines before).
  • I think there was another potential bug, where _get_packages was used elsewhere, and only with a very old pip, as _get_packages was not adding pip and setuptools to the list, if the installation was to install pip or setuptools, it may get unnoticed by changed = out_freeze_before != out_freeze_after.

The whole thing clearly lacks tests, I only tested it myself and it worked as expected (--check telling a module should be updated, running the playbook upgrades the module, --check again tells it's OK), but I'm out of time for today and have no idea how to test it for now (any hint appreciated).

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

pip

@ansibot
Copy link
Contributor

ansibot commented Oct 22, 2020

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/pip.py:374:59: undefined-variable: Undefined variable 'env'
lib/ansible/modules/pip.py:376:16: undefined-variable: Undefined variable 'pkg_list'

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/pip.py:401:50: SyntaxError: return command, out, err, {package['name'] for package in json.loads(out)}

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/pip.py:401:50: traceback: SyntaxError: invalid syntax

click here for bot help

@ansibot ansibot added affects_2.11 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. packaging Packaging category support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Oct 22, 2020
@JulienPalard
Copy link
Contributor Author

Python 2.6.

@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 Oct 22, 2020
@ansibot
Copy link
Contributor

ansibot commented Oct 22, 2020

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/pip.py:400:50: SyntaxError: return command, out, err, {package['name'] for package in json.loads(out)}

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/pip.py:400:50: traceback: SyntaxError: invalid syntax

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 Oct 22, 2020
@relrod relrod removed the needs_triage Needs a first human triage before being processed. label Oct 22, 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 Oct 22, 2020
@ansibot
Copy link
Contributor

ansibot commented Oct 22, 2020

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/pip.py:761:68: SyntaxError: if not outdated.isdisjoint({package.package_name for package in packages}):

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/pip.py:761:68: traceback: SyntaxError: invalid syntax

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 Oct 22, 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 Oct 22, 2020
@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 Oct 31, 2020
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. pre_azp This PR was last tested before migration to Azure Pipelines. and removed core_review In order to be merged, this PR must follow the core review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 10, 2020
@ArsenArsen
Copy link

Related to this, state=latest outside of check mode never returns changed, despite possibly reinstalling a package.

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Sep 5, 2021
@jborean93
Copy link
Contributor

Closing due to inactivity. If this is still something you wish to pursue then please open a new PR. We expect tests to be added for any changes made that fixes issues or updates the behaviour.

@jborean93 jborean93 closed this Jun 8, 2022
@ansible ansible locked and limited conversation to collaborators Jun 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.11 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. packaging Packaging category pre_azp This PR was last tested before migration to Azure Pipelines. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip module always set to changed when using check mode
6 participants