From 55c04706de5dce1fb8ce24e1b27dacf43b1be6ce Mon Sep 17 00:00:00 2001 From: Davide Sbetti Date: Fri, 29 Sep 2023 17:33:47 +0200 Subject: [PATCH 1/8] Consider the output of both pip list and freeze, handle forcereinstall --- lib/ansible/modules/pip.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/ansible/modules/pip.py b/lib/ansible/modules/pip.py index fa105bbef21..d7fdc230dbf 100644 --- a/lib/ansible/modules/pip.py +++ b/lib/ansible/modules/pip.py @@ -392,19 +392,20 @@ def _get_cmd_options(module, cmd): def _get_packages(module, pip, chdir): '''Return results of pip command to get packages.''' # Try 'pip list' command first. - command = pip + ['list', '--format=freeze'] + list_command = pip + ['list', '--format=freeze'] locale = get_best_parsable_locale(module) lang_env = {'LANG': locale, 'LC_ALL': locale, 'LC_MESSAGES': locale} - rc, out, err = module.run_command(command, cwd=chdir, environ_update=lang_env) + _rc, list_out, list_err = module.run_command(list_command, cwd=chdir, environ_update=lang_env) - # If there was an error (pip version too old) then use 'pip freeze'. + # Use also pip freeze, since pip list does not show the commit used + # in case of installation from VCS. + # We keep the already existing behaviour of failing only if the freeze command fails + freeze_command = pip + ['freeze'] + rc, freeze_out, freeze_err = module.run_command(freeze_command, cwd=chdir) if rc != 0: - command = pip + ['freeze'] - rc, out, err = module.run_command(command, cwd=chdir) - if rc != 0: - _fail(module, command, out, err) + _fail(module, freeze_command, freeze_out, freeze_err) - return ' '.join(command), out, err + return ' '.join(list_command + [";"] + freeze_command), list_out + freeze_out, list_err + freeze_err def _is_present(module, req, installed_pkgs, pkg_command): @@ -799,7 +800,7 @@ def main(): ) if module.check_mode: - if extra_args or requirements or state == 'latest' or not name: + if extra_args or requirements or state == 'latest' or not name or state== "forcereinstall": module.exit_json(changed=True) pkg_cmd, out_pip, err_pip = _get_packages(module, pip, chdir) @@ -849,7 +850,7 @@ def main(): changed = 'Successfully installed' in out_pip else: dummy, out_freeze_after, dummy = _get_packages(module, pip, chdir) - changed = out_freeze_before != out_freeze_after + changed = out_freeze_before != out_freeze_after or state == "forcereinstall" changed = changed or venv_created From 13f62496387a8ae68bd1b3e05dfdf239bf2a91f5 Mon Sep 17 00:00:00 2001 From: Davide Sbetti Date: Sat, 30 Sep 2023 11:08:34 +0200 Subject: [PATCH 2/8] Add tests for force-reinstall and VCS installation commit evaluation --- test/integration/targets/pip/tasks/pip.yml | 64 ++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/test/integration/targets/pip/tasks/pip.yml b/test/integration/targets/pip/tasks/pip.yml index 9f1034d29a9..f02e680ffca 100644 --- a/test/integration/targets/pip/tasks/pip.yml +++ b/test/integration/targets/pip/tasks/pip.yml @@ -63,6 +63,31 @@ - command: "{{ ansible_python.executable }} -c 'import {{ item }}'" loop: '{{ pip_test_modules }}' +# force reinstall packages in check mode and ensure we recorded a change +- name: forcereinstall packages in check mode + pip: + name: "{{ pip_test_packages }}" + state: forcereinstall + check_mode: true + register: forcereinstall_result_check_mode + +- name: verify we recorded a change + assert: + that: + - "forcereinstall_result_check_mode is changed" + +# force reinstall packages for real and ensure we recorded a change +- name: forcereinstall packages + pip: + name: "{{ pip_test_packages }}" + state: forcereinstall + register: forcereinstall_result + +- name: verify we recorded a change + assert: + that: + - "forcereinstall_result is changed" + # now remove it to test uninstallation of a package we are sure is installed - name: now uninstall so we can see that a change occurred @@ -129,6 +154,45 @@ that: - "not (url_installed is changed)" +- name: install the same module from url pointing to a specific commit + pip: + name: "git+https://github.com/dvarrazzo/pyiso8601@8bfaaa3e5c63a9eda4449e606786802f4e95ba60#egg=iso8601" + virtualenv: "{{ remote_tmp_dir }}/pipenv" + editable: true + state: latest + register: url_installed_specific_commit + +- name: verify we recorded a change + assert: + that: + - "url_installed_specific_commit is changed" + +- name: install the same module from url pointing to the same specific commit + pip: + name: "git+https://github.com/dvarrazzo/pyiso8601@8bfaaa3e5c63a9eda4449e606786802f4e95ba60#egg=iso8601" + virtualenv: "{{ remote_tmp_dir }}/pipenv" + editable: true + state: latest + register: url_installed_specific_commit_again + +- name: verify we did not record a change + assert: + that: + - "not (url_installed_specific_commit_again is changed)" + +- name: install the same module from url pointing to a different specific commit with same package version + pip: + name: "git+https://github.com/dvarrazzo/pyiso8601@a48aa33f9fe5aa77d40fa2a38584750570d38ad6#egg=iso8601" + state: latest + virtualenv: "{{ remote_tmp_dir }}/pipenv" + register: url_installed_different_specific_commit_same_version + +- name: verify we recorded a change + assert: + that: + - "url_installed_different_specific_commit_same_version is changed" + + # Test pip package in check mode doesn't always report changed. # Special case for pip From 2c8c4e5927a588fb90b6e9dd61e7c30051dde417 Mon Sep 17 00:00:00 2001 From: Davide Sbetti Date: Sat, 30 Sep 2023 11:29:44 +0200 Subject: [PATCH 3/8] Fix whitespace, add editable in test --- lib/ansible/modules/pip.py | 2 +- test/integration/targets/pip/tasks/pip.yml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/ansible/modules/pip.py b/lib/ansible/modules/pip.py index d7fdc230dbf..f2222b89f9b 100644 --- a/lib/ansible/modules/pip.py +++ b/lib/ansible/modules/pip.py @@ -800,7 +800,7 @@ def main(): ) if module.check_mode: - if extra_args or requirements or state == 'latest' or not name or state== "forcereinstall": + if extra_args or requirements or state == 'latest' or not name or state == "forcereinstall": module.exit_json(changed=True) pkg_cmd, out_pip, err_pip = _get_packages(module, pip, chdir) diff --git a/test/integration/targets/pip/tasks/pip.yml b/test/integration/targets/pip/tasks/pip.yml index f02e680ffca..93fdd603171 100644 --- a/test/integration/targets/pip/tasks/pip.yml +++ b/test/integration/targets/pip/tasks/pip.yml @@ -184,6 +184,7 @@ pip: name: "git+https://github.com/dvarrazzo/pyiso8601@a48aa33f9fe5aa77d40fa2a38584750570d38ad6#egg=iso8601" state: latest + editable: true virtualenv: "{{ remote_tmp_dir }}/pipenv" register: url_installed_different_specific_commit_same_version From b4734a956cdb4a08eb37b22d1fb757a1ead96efb Mon Sep 17 00:00:00 2001 From: Davide Sbetti Date: Sat, 30 Sep 2023 11:39:55 +0200 Subject: [PATCH 4/8] Revert potential incorrect fix for forcereinstall, add changelog fragment --- ...815-fix-pip-changed-status-vcs-install.yml | 2 ++ lib/ansible/modules/pip.py | 4 +-- test/integration/targets/pip/tasks/pip.yml | 25 ------------------- 3 files changed, 4 insertions(+), 27 deletions(-) create mode 100644 changelogs/fragments/81815-fix-pip-changed-status-vcs-install.yml diff --git a/changelogs/fragments/81815-fix-pip-changed-status-vcs-install.yml b/changelogs/fragments/81815-fix-pip-changed-status-vcs-install.yml new file mode 100644 index 00000000000..5d74b964812 --- /dev/null +++ b/changelogs/fragments/81815-fix-pip-changed-status-vcs-install.yml @@ -0,0 +1,2 @@ +bugfixes: + - pip - Incorrect changed status was returned in case of installation from VCS where the target commit was different but the version was the same. The fix now evaluates also the commits used for installation from VCS (https://github.com/ansible/ansible/issues/81751). \ No newline at end of file diff --git a/lib/ansible/modules/pip.py b/lib/ansible/modules/pip.py index f2222b89f9b..ca80e90ff21 100644 --- a/lib/ansible/modules/pip.py +++ b/lib/ansible/modules/pip.py @@ -800,7 +800,7 @@ def main(): ) if module.check_mode: - if extra_args or requirements or state == 'latest' or not name or state == "forcereinstall": + if extra_args or requirements or state == 'latest' or not name: module.exit_json(changed=True) pkg_cmd, out_pip, err_pip = _get_packages(module, pip, chdir) @@ -850,7 +850,7 @@ def main(): changed = 'Successfully installed' in out_pip else: dummy, out_freeze_after, dummy = _get_packages(module, pip, chdir) - changed = out_freeze_before != out_freeze_after or state == "forcereinstall" + changed = out_freeze_before != out_freeze_after changed = changed or venv_created diff --git a/test/integration/targets/pip/tasks/pip.yml b/test/integration/targets/pip/tasks/pip.yml index 93fdd603171..6b3a320c1b5 100644 --- a/test/integration/targets/pip/tasks/pip.yml +++ b/test/integration/targets/pip/tasks/pip.yml @@ -63,31 +63,6 @@ - command: "{{ ansible_python.executable }} -c 'import {{ item }}'" loop: '{{ pip_test_modules }}' -# force reinstall packages in check mode and ensure we recorded a change -- name: forcereinstall packages in check mode - pip: - name: "{{ pip_test_packages }}" - state: forcereinstall - check_mode: true - register: forcereinstall_result_check_mode - -- name: verify we recorded a change - assert: - that: - - "forcereinstall_result_check_mode is changed" - -# force reinstall packages for real and ensure we recorded a change -- name: forcereinstall packages - pip: - name: "{{ pip_test_packages }}" - state: forcereinstall - register: forcereinstall_result - -- name: verify we recorded a change - assert: - that: - - "forcereinstall_result is changed" - # now remove it to test uninstallation of a package we are sure is installed - name: now uninstall so we can see that a change occurred From 2fa74acd810eba2824b7a77973c4b920bdf1a756 Mon Sep 17 00:00:00 2001 From: Davide Sbetti Date: Sat, 30 Sep 2023 11:52:54 +0200 Subject: [PATCH 5/8] Fix changelog fragment name --- .../fragments/81816-fix-pip-changed-status-vcs-install.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/81816-fix-pip-changed-status-vcs-install.yml diff --git a/changelogs/fragments/81816-fix-pip-changed-status-vcs-install.yml b/changelogs/fragments/81816-fix-pip-changed-status-vcs-install.yml new file mode 100644 index 00000000000..5d74b964812 --- /dev/null +++ b/changelogs/fragments/81816-fix-pip-changed-status-vcs-install.yml @@ -0,0 +1,2 @@ +bugfixes: + - pip - Incorrect changed status was returned in case of installation from VCS where the target commit was different but the version was the same. The fix now evaluates also the commits used for installation from VCS (https://github.com/ansible/ansible/issues/81751). \ No newline at end of file From 9ab3839e4b9bf0891a377e2531996375ad3fb69d Mon Sep 17 00:00:00 2001 From: Davide Sbetti Date: Sat, 30 Sep 2023 11:54:59 +0200 Subject: [PATCH 6/8] Remove wrong changelog fragment --- .../fragments/81815-fix-pip-changed-status-vcs-install.yml | 2 -- 1 file changed, 2 deletions(-) delete mode 100644 changelogs/fragments/81815-fix-pip-changed-status-vcs-install.yml diff --git a/changelogs/fragments/81815-fix-pip-changed-status-vcs-install.yml b/changelogs/fragments/81815-fix-pip-changed-status-vcs-install.yml deleted file mode 100644 index 5d74b964812..00000000000 --- a/changelogs/fragments/81815-fix-pip-changed-status-vcs-install.yml +++ /dev/null @@ -1,2 +0,0 @@ -bugfixes: - - pip - Incorrect changed status was returned in case of installation from VCS where the target commit was different but the version was the same. The fix now evaluates also the commits used for installation from VCS (https://github.com/ansible/ansible/issues/81751). \ No newline at end of file From 22ad8650e53c943f80eb07a2ee81ea5d424ec906 Mon Sep 17 00:00:00 2001 From: Davide Sbetti Date: Sat, 30 Sep 2023 12:33:16 +0200 Subject: [PATCH 7/8] Fetch commits in test to overcome old pip bug for later installations --- test/integration/targets/pip/tasks/pip.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/integration/targets/pip/tasks/pip.yml b/test/integration/targets/pip/tasks/pip.yml index 6b3a320c1b5..b5d23c76e55 100644 --- a/test/integration/targets/pip/tasks/pip.yml +++ b/test/integration/targets/pip/tasks/pip.yml @@ -129,6 +129,14 @@ that: - "not (url_installed is changed)" +# Note: this is due to a bug of pip which in old version does not automatically refetch +# refs after the first install +- name: Fetch commits needed for following test + command: git fetch origin 8bfaaa3e5c63a9eda4449e606786802f4e95ba60 a48aa33f9fe5aa77d40fa2a38584750570d38ad6 + args: + chdir: "{{ remote_tmp_dir }}/pipenv/src/iso8601/" + + - name: install the same module from url pointing to a specific commit pip: name: "git+https://github.com/dvarrazzo/pyiso8601@8bfaaa3e5c63a9eda4449e606786802f4e95ba60#egg=iso8601" From 880e68210f2926ea15318ae1ef8a61e588d797b7 Mon Sep 17 00:00:00 2001 From: Davide Sbetti Date: Sat, 30 Sep 2023 19:55:08 +0200 Subject: [PATCH 8/8] Fix test for pip 9.0.3 on python3.6 --- test/integration/targets/pip/tasks/pip.yml | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/test/integration/targets/pip/tasks/pip.yml b/test/integration/targets/pip/tasks/pip.yml index b5d23c76e55..b2994a1ac19 100644 --- a/test/integration/targets/pip/tasks/pip.yml +++ b/test/integration/targets/pip/tasks/pip.yml @@ -129,17 +129,16 @@ that: - "not (url_installed is changed)" -# Note: this is due to a bug of pip which in old version does not automatically refetch -# refs after the first install -- name: Fetch commits needed for following test - command: git fetch origin 8bfaaa3e5c63a9eda4449e606786802f4e95ba60 a48aa33f9fe5aa77d40fa2a38584750570d38ad6 - args: +# Note: this is needed due to an issue of pip 9.0.3 and python 3.6 to install +# non refs commit (reproduced on RHEL 8.8 trying to run 'pip3 install -e git+https://github.com/dvarrazzo/pyiso8601@8bfaaa3e5c63a9eda4449e606786802f4e95ba60#egg=iso8601') +- name: update git refs for backcompatibility with pip 9.0.3 + shell: "git update-ref refs/heads/test 8bfaaa3e5c63a9eda4449e606786802f4e95ba60; git update-ref refs/heads/test1 a48aa33f9fe5aa77d40fa2a38584750570d38ad6" + args: chdir: "{{ remote_tmp_dir }}/pipenv/src/iso8601/" - - name: install the same module from url pointing to a specific commit pip: - name: "git+https://github.com/dvarrazzo/pyiso8601@8bfaaa3e5c63a9eda4449e606786802f4e95ba60#egg=iso8601" + name: "git+https://github.com/dvarrazzo/pyiso8601@test#egg=iso8601" virtualenv: "{{ remote_tmp_dir }}/pipenv" editable: true state: latest @@ -152,7 +151,7 @@ - name: install the same module from url pointing to the same specific commit pip: - name: "git+https://github.com/dvarrazzo/pyiso8601@8bfaaa3e5c63a9eda4449e606786802f4e95ba60#egg=iso8601" + name: "git+https://github.com/dvarrazzo/pyiso8601@test#egg=iso8601" virtualenv: "{{ remote_tmp_dir }}/pipenv" editable: true state: latest @@ -165,7 +164,7 @@ - name: install the same module from url pointing to a different specific commit with same package version pip: - name: "git+https://github.com/dvarrazzo/pyiso8601@a48aa33f9fe5aa77d40fa2a38584750570d38ad6#egg=iso8601" + name: "git+https://github.com/dvarrazzo/pyiso8601@test1#egg=iso8601" state: latest editable: true virtualenv: "{{ remote_tmp_dir }}/pipenv"