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

apt module : take care about autoremove in upgrade function #30747

Merged
merged 1 commit into from
Oct 2, 2017
Merged

apt module : take care about autoremove in upgrade function #30747

merged 1 commit into from
Oct 2, 2017

Conversation

dj4ngo
Copy link
Contributor

@dj4ngo dj4ngo commented Sep 22, 2017

SUMMARY

Fixes #30516
Take care about params autoremove in upgrade function.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

packaging/os/apt

ANSIBLE VERSION
ansible 2.5.0 (issue_30516 671de05a33) last updated 2017/09/22 13:51:54 (GMT +200)

@ansibot
Copy link
Contributor

ansibot commented Sep 22, 2017

cc @mgwilliams
click here for bot help

@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bugfix_pull_request core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 22, 2017
@mscherer mscherer removed the needs_triage Needs a first human triage before being processed. label Sep 22, 2017
@mscherer
Copy link
Contributor

shipit

@dj4ngo dj4ngo changed the title [fix] issue #30516 : take care about autoremove in upgrade function [fix] issue #30516 : apt module : take care about autoremove in upgrade function Sep 24, 2017
@dj4ngo
Copy link
Contributor Author

dj4ngo commented Sep 25, 2017

cc @kbrebanov @elasticdog, I ping you as namespace maintener, to review this PR since mgwilliams seems not available.

@kbrebanov
Copy link
Contributor

This would not change the functionality, but as a matter of consistency with the rest of the code, what would you think of passing the autoremove parameter when calling the upgrade function instead of looking up the value thru the module parameters?

Like when the install and remove functions are called.

@dj4ngo
Copy link
Contributor Author

dj4ngo commented Sep 26, 2017

@kbrebanov I was thinking about it but I don't really agree with how args are managed...
You're right, I'll push the change you are talking about for this PR

But I would like to talk a little bit about attributes management in this module.
Since we get the ansible module m, we got two choices:

  • to use it in each function to set the apt args to use (eg --autoremove), but this need to perform all if tests in each functions...
  • create a function or code factorization to generate all cli args (with --autoremove in our case)
    We could create a dictionary containing all the cli args for cmd (empty or with value) and pass it to each function to avoid having to modify args each-time we implement new functionnality.
    I'll propose a feature to implement this.

@kbrebanov
Copy link
Contributor

@dj4ngo I think the upgrade call on line 984 may also need to be modified.

I agree with you that attributes management could probably be improved.

@dj4ngo
Copy link
Contributor Author

dj4ngo commented Sep 26, 2017 •
edited

@kbrebanov your right, I did a missmatch in my amend .... I amend again my commit.

@kbrebanov
Copy link
Contributor

shipit

@dj4ngo
Copy link
Contributor Author

dj4ngo commented Sep 27, 2017

@mscherer Could you review this PR again ? I had to modify it since your shipit message ... Thanks

mscherer
mscherer reviewed Sep 30, 2017
View reviewed changes
lib/ansible/modules/packaging/os/apt.py
@@ -722,7 +722,13 @@ def cleanup(m, purge=False, force=False, operation=None,

def upgrade(m, mode="yes", force=False, default_release=None,
use_apt_get=False,
dpkg_options=expand_dpkg_options(DPKG_OPTIONS)):
dpkg_options=expand_dpkg_options(DPKG_OPTIONS), autoremove=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the default for autoremove be "False" rather than "None" ? (just nitpicking)

@mscherer
Copy link
Contributor

shipit

@mscherer
Copy link
Contributor

I suspect we are missing a few fixes. For example, what happen if there is no aptitude, but someone explictely say "autoremove=yes", the module should fail and/or warn the user, no ?

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. labels Sep 30, 2017
@bcoca bcoca merged commit af3e895 into ansible:devel Oct 2, 2017
@Nothing4You Nothing4You mentioned this pull request Oct 22, 2017
Closed
@alikins alikins added this to Nominated in 2.4.x Blocker List Oct 23, 2017
@bcoca bcoca changed the title [fix] issue #30516 : apt module : take care about autoremove in upgrade function apt module : take care about autoremove in upgrade function Oct 23, 2017
@abadger abadger moved this from Nominated to TODO: Nice to have in 2.4.x Blocker List Oct 23, 2017
@abadger
Copy link
Contributor

abadger commented Oct 26, 2017

cherry-picked for 2.4.2beta1

@abadger abadger moved this from TODO: Nice to have to Done in 2.4.2 in 2.4.x Blocker List Oct 26, 2017
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018
@mkrizek mkrizek mentioned this pull request Mar 19, 2018
Merged
@dagwieers dagwieers added the packaging Packaging category label Mar 3, 2019
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mscherer mscherer mscherer left review comments

Assignees
No one assigned
Labels
affects_2.5 This issue/PR affects Ansible v2.5 bug This issue/PR relates to a bug. module This issue/PR relates to a module. packaging Packaging category shipit This PR is ready to be merged by Core support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
No open projects
2.4.x Blocker List
Done in 2.4.2
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

apt module uses apt-get --autoremove even if autoremove=no
7 participants
@dj4ngo @ansibot @mscherer @kbrebanov @abadger @dagwieers @bcoca