Repository: ansible/ansible
Pull request: apt: Report change when package is removed
##### SUMMARY

While upgrade process removes a package, module should
report changed=True instead of changed=False

Fixes: #46314

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>

##### ISSUE TYPE
- Bugfix Pull Request




Problem statement:
Using apt module, something changes but changed remains false and the change seems undetected
<!--- Verify first that your issue is not already reported on GitHub -->
<!--- Also test if the latest release and devel branch are affected too -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
When using the apt module against multiple machines, I noticed in the output a change has taken place but the status for 'changed' is equal to false, so the change is not detected and reports as green not yellow. Not sure if this is a bug or not. 

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
apt

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```
$ ansible --version
ansible 2.6.4
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/patrick/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/dist-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.15rc1 (default, Apr 15 2018, 21:51:34) [GCC 7.3.0]

```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```
$ ansible-config dump --only-changed
$

```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->
Ubuntu 18.04 ESXi VM , all latest updates installed

##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->
The following command was run and the output for the machine is below
You will note changed = false however an action has been performed

<!--- Paste example playbooks or commands between quotes below -->
```
$ ansible all -m apt -a "update_cache=yes cache_valid_time=86400 upgrade=yes autoremove=yes" --become

n40l.x | SUCCESS => {
    "changed": false,
    "msg": "Reading package lists...\nBuilding dependency tree...\nReading state information...\nCalculating upgrade...\nThe following packages will be REMOVED:\n  libio-string-perl\n0 upgraded, 0 newly installed, 1 to remove and 0 not upgraded.\nAfter this operation, 59.4 kB disk space will be freed.\n(Reading database ... \r(Reading database ... 5%\r(Reading database ... 10%\r(Reading database ... 15%\r(Reading database ... 20%\r(Reading database ... 25%\r(Reading database ... 30%\r(Reading database ... 35%\r(Reading database ... 40%\r(Reading database ... 45%\r(Reading database ... 50%\r(Reading database ... 55%\r(Reading database ... 60%\r(Reading database ... 65%\r(Reading database ... 70%\r(Reading database ... 75%\r(Reading database ... 80%\r(Reading database ... 85%\r(Reading database ... 90%\r(Reading database ... 95%\r(Reading database ... 100%\r(Reading database ... 84020 files and directories currently installed.)\r\nRemoving libio-string-perl (1.08-3) ...\r\nProcessing triggers for man-db (2.7.5-1) ...\r\n",
    "stderr": "",
    "stderr_lines": [],
    "stdout": "Reading package lists...\nBuilding dependency tree...\nReading state information...\nCalculating upgrade...\nThe following packages will be REMOVED:\n  libio-string-perl\n0 upgraded, 0 newly installed, 1 to remove and 0 not upgraded.\nAfter this operation, 59.4 kB disk space will be freed.\n(Reading database ... \r(Reading database ... 5%\r(Reading database ... 10%\r(Reading database ... 15%\r(Reading database ... 20%\r(Reading database ... 25%\r(Reading database ... 30%\r(Reading database ... 35%\r(Reading database ... 40%\r(Reading database ... 45%\r(Reading database ... 50%\r(Reading database ... 55%\r(Reading database ... 60%\r(Reading database ... 65%\r(Reading database ... 70%\r(Reading database ... 75%\r(Reading database ... 80%\r(Reading database ... 85%\r(Reading database ... 90%\r(Reading database ... 95%\r(Reading database ... 100%\r(Reading database ... 84020 files and directories currently installed.)\r\nRemoving libio-string-perl (1.08-3) ...\r\nProcessing triggers for man-db (2.7.5-1) ...\r\n",
    "stdout_lines": [
        "Reading package lists...",
        "Building dependency tree...",
        "Reading state information...",
        "Calculating upgrade...",
        "The following packages will be REMOVED:",
        "  libio-string-perl",
        "0 upgraded, 0 newly installed, 1 to remove and 0 not upgraded.",
        "After this operation, 59.4 kB disk space will be freed.",
        "(Reading database ... ",
        "(Reading database ... 5%",
        "(Reading database ... 10%",
        "(Reading database ... 15%",
        "(Reading database ... 20%",
        "(Reading database ... 25%",
        "(Reading database ... 30%",
        "(Reading database ... 35%",
        "(Reading database ... 40%",
        "(Reading database ... 45%",
        "(Reading database ... 50%",
        "(Reading database ... 55%",
        "(Reading database ... 60%",
        "(Reading database ... 65%",
        "(Reading database ... 70%",
        "(Reading database ... 75%",
        "(Reading database ... 80%",
        "(Reading database ... 85%",
        "(Reading database ... 90%",
        "(Reading database ... 95%",
        "(Reading database ... 100%",
        "(Reading database ... 84020 files and directories currently installed.)",
        "Removing libio-string-perl (1.08-3) ...",
        "Processing triggers for man-db (2.7.5-1) ..."
    ]
}

```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
I would expect changed = true and the result to show as yellow not green in the results

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->
A change was made, but changed remained false and the result was shown as green not yellow

<!--- Paste verbatim command output between quotes -->
```paste below

```

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/packaging/os/apt.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/packaging/os/apt.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @mgwilliams
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
autoremove seems to not be correctly showing as 'changed'
Files identified in the description:
* [`lib/ansible/modules/apt.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/apt.py)

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->

Commit message: Fixed ubuntu 2204 test again

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>

Patch to review:
diff --git a/changelogs/fragments/46314.yml b/changelogs/fragments/46314.yml
new file mode 100644
index 00000000000000..9958061e8f61c0
--- /dev/null
+++ b/changelogs/fragments/46314.yml
@@ -0,0 +1,3 @@
+---
+bugfixes:
+  - apt - report changed=True when some packages are being removed (https://github.com/ansible/ansible/issues/46314).
diff --git a/lib/ansible/modules/apt.py b/lib/ansible/modules/apt.py
index 0b496dfb89bc30..59e91949751cf0 100644
--- a/lib/ansible/modules/apt.py
+++ b/lib/ansible/modules/apt.py
@@ -381,8 +381,8 @@
 from ansible.module_utils.urls import fetch_file
 
 DPKG_OPTIONS = 'force-confdef,force-confold'
-APT_GET_ZERO = "\n0 upgraded, 0 newly installed"
-APTITUDE_ZERO = "\n0 packages upgraded, 0 newly installed"
+APT_GET_ZERO = "\n0 upgraded, 0 newly installed, 0 to remove"
+APTITUDE_ZERO = "\n0 packages upgraded, 0 newly installed, 0 to remove"
 APT_LISTS_PATH = "/var/lib/apt/lists"
 APT_UPDATE_SUCCESS_STAMP_PATH = "/var/lib/apt/periodic/update-success-stamp"
 APT_MARK_INVALID_OP = 'Invalid operation'
diff --git a/test/integration/targets/apt/tasks/repo.yml b/test/integration/targets/apt/tasks/repo.yml
index b1d08afa34223e..70e2e6a286c17d 100644
--- a/test/integration/targets/apt/tasks/repo.yml
+++ b/test/integration/targets/apt/tasks/repo.yml
@@ -384,6 +384,8 @@
         - { upgrade_type: safe, force_apt_get: True }
         - { upgrade_type: full, force_apt_get: True }
 
+    - include_tasks: "upgrade_autoremove.yml"
+
     - name: (Re-)Install aptitude, run same tests again
       apt:
         pkg: aptitude
@@ -401,6 +403,7 @@
         - { upgrade_type: full, force_apt_get: True }
 
     - include_tasks: "upgrade_scenarios.yml"
+    - include_tasks: "upgrade_autoremove.yml"
 
     - name: Remove aptitude if not originally present
       apt:
diff --git a/test/integration/targets/apt/tasks/upgrade_autoremove.yml b/test/integration/targets/apt/tasks/upgrade_autoremove.yml
new file mode 100644
index 00000000000000..a05b81c2fc352e
--- /dev/null
+++ b/test/integration/targets/apt/tasks/upgrade_autoremove.yml
@@ -0,0 +1,71 @@
+# https://github.com/ansible/ansible/issues/46314
+- block:
+    - name: Remove upgrades from the equation
+      apt:
+        upgrade: true
+        state: present
+        update_cache: true
+
+    - name: Install foobar, installs foo as a dependency
+      apt:
+        name: foobar=1.0.0
+        allow_unauthenticated: true
+
+    - name: Check foobar version
+      shell: dpkg -s foobar | grep Version | awk '{print $2}'
+      register: foobar_version
+
+    - name: Ensure the correct version of foobar has been installed
+      assert:
+        that:
+          - "'1.0.0' in foobar_version.stdout"
+
+    - name: test autoremove + upgrade (check mode)
+      apt:
+        autoremove: true
+        upgrade: true
+      diff: true
+      check_mode: true
+      register: autoremove_check_mode
+
+    - name: test autoremove + upgrade
+      apt:
+        autoremove: true
+        upgrade: true
+      diff: true
+      register: autoremove
+
+    - name: Check that something is changed
+      assert:
+        that:
+          - autoremove.changed
+          - autoremove_check_mode.changed
+
+    - name: check foo version
+      shell: dpkg -s foobar | grep Version | awk '{print $2}'
+      register: foobar_version
+
+    - name: check that old version upgraded correctly
+      assert:
+        that:
+          - "'1.0.0' not in foobar_version.stdout"
+          - "{{ foobar_version.changed }}"
+
+    - name: test autoremove + upgrade (Idempotant)
+      apt:
+        autoremove: true
+        upgrade: true
+      diff: true
+      register: second_upgrade_result
+
+    - name: check that nothing has changed (Idempotant)
+      assert:
+        that:
+          - "second_upgrade_result.changed == false"
+
+  always:
+    - name: Clean up
+      apt:
+        pkg: foo,foobar
+        state: absent
+        autoclean: true


Code review comments:
diff_hunk: @@ -0,0 +1,71 @@
+# https://github.com/ansible/ansible/issues/46314
+- block:
+    - name: Remove upgrades from the equation
+      apt:
+        upgrade: true
+        state: present
+        update_cache: true
+
+    - name: Install foobar, installs foo as a dependency
+      apt:
+        name: foobar=1.0.0
+        allow_unauthenticated: true
+
+    - name: Check foobar version
+      shell: dpkg -s foobar | grep Version | awk '{print $2}'
+      register: foobar_version
+
+    - name: Ensure the correct version of foobar has been installed
+      assert:
+        that:
+          - "'1.0.0' in foobar_version.stdout"
+
+    - name: test autoremove + upgrade (check mode)
path: test/integration/targets/apt/tasks/upgrade_autoremove.yml
text: @user1:
This task upgrades foobar instead of removing the unused dependency foo, and changed is reported correctly with or without the patch. Writing tests before applying the patch can help ensure it will prevent a regression.

```suggestion
    - name: Remove foobar, leaving behind its dependency foo
      apt:
        name: foobar=1.0.0
        state: absent

    - name: test autoremove + upgrade (check mode)
```

@author:
Good catch. Thanks.

diff_hunk: @@ -0,0 +1,76 @@
+# https://github.com/ansible/ansible/issues/46314
+- block:
+    - name: Remove upgrades from the equation
+      apt:
+        upgrade: true
+        state: present
+        update_cache: true
+
+    - name: Install foobar, installs foo as a dependency
+      apt:
+        name: foobar=1.0.0
+        allow_unauthenticated: true
+
+    - name: Check foobar version
+      shell: dpkg -s foobar | grep Version | awk '{print $2}'
+      register: foobar_version
+
+    - name: Ensure the correct version of foobar has been installed
+      assert:
+        that:
+          - "'1.0.0' in foobar_version.stdout"
+
+    - name: Remove foobar, leaving behind its dependency foo
+      apt:
+        name: foobar=1.0.0
+        state: absent
+
+    - name: test autoremove + upgrade (check mode)
+      apt:
+        autoremove: true
+        upgrade: true
+      diff: true
+      check_mode: true
+      register: autoremove_check_mode
+
+    - name: test autoremove + upgrade
+      apt:
+        autoremove: true
+        upgrade: true
+      diff: true
+      register: autoremove
+
+    - name: Check that something is changed
+      assert:
+        that:
+          - autoremove.changed
+          - autoremove_check_mode.changed
+
+    - name: check foo version
+      shell: dpkg -s foobar | grep Version | awk '{print $2}'
+      register: foobar_version
+
+    - name: check that old version upgraded correctly
+      assert:
+        that:
+          - "'1.0.0' not in foobar_version.stdout"
path: test/integration/targets/apt/tasks/upgrade_autoremove.yml
text: @user1:
```suggestion
          - "'1.0.1' not in foobar_version.stdout"
```

I didn't notice the name of the task, but that is outdated too.

diff_hunk: @@ -0,0 +1,76 @@
+# https://github.com/ansible/ansible/issues/46314
+- block:
+    - name: Remove upgrades from the equation
+      apt:
+        upgrade: true
+        state: present
+        update_cache: true
+
+    - name: Install foobar, installs foo as a dependency
+      apt:
+        name: foobar=1.0.0
+        allow_unauthenticated: true
+
+    - name: Check foobar version
+      shell: dpkg -s foobar | grep Version | awk '{print $2}'
+      register: foobar_version
+
+    - name: Ensure the correct version of foobar has been installed
+      assert:
+        that:
+          - "'1.0.0' in foobar_version.stdout"
+
+    - name: Remove foobar, leaving behind its dependency foo
+      apt:
+        name: foobar=1.0.0
+        state: absent
+
+    - name: test autoremove + upgrade (check mode)
+      apt:
+        autoremove: true
+        upgrade: true
+      diff: true
+      check_mode: true
+      register: autoremove_check_mode
+
+    - name: test autoremove + upgrade
+      apt:
+        autoremove: true
+        upgrade: true
+      diff: true
+      register: autoremove
+
+    - name: Check that something is changed
+      assert:
+        that:
+          - autoremove.changed
+          - autoremove_check_mode.changed
+
+    - name: check foo version
+      shell: dpkg -s foobar | grep Version | awk '{print $2}'
path: test/integration/targets/apt/tasks/upgrade_autoremove.yml
text: @user1:
```suggestion
      shell: dpkg -s foo | grep Version | awk '{print $2}'
```