ansible-collections / community.general

Ansible Community General Collection
https://galaxy.ansible.com/ui/repo/published/community/general/
GNU General Public License v3.0
811 stars 1.49k forks source link

Zypper `disable_recommends` default setting violates principle of least surprise #3497

Open je2555 opened 2 years ago

je2555 commented 2 years ago

Summary

The default behaviour of the zypper package manager is to install a package and its recommended packages. A user has to explicitly override that with the --no-recommends command line flag or with the solver.onlyRequires = true configuration setting. However the community.general.zypper module has the default behaviour reversed, disabling recommended packages by default.

This violates the principle of least surprise. A user shouldn't need to refer to the specific documentation to see what defaults have been changed, zypper from within ansible should function the same as zypper on the command line.

If we compare the zypper module with the apt or yum modules we can see the difference:

Ansible Module OS Default Ansible Default
apt install recommends honour the OS configuration
dnf/yum install weak deps install weak deps
zypper install recommends don't install recommends

I can submit a pull request if you will accept the change.

Issue Type

Bug Report

Component Name

zypper

Ansible Version

$ ansible --version
ansible [core 2.11.5] 
  config file = None
  configured module search path = ['/home/myhomedir/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/myhomedir/Documents/Setup/init_tumbleweed/lib64/python3.8/site-packages/ansible
  ansible collection location = /home/myhomedir/.ansible/collections:/usr/share/ansible/collections
  executable location = /home/myhomedir/Documents/Setup/init_tumbleweed/bin/ansible
  python version = 3.8.12 (default, Aug 31 2021, 01:23:42) [GCC]
  jinja version = 3.0.1
  libyaml = True

Community.general Version

$ ansible-galaxy collection list community.general

# /home/myhomedir/Documents/Setup/init_tumbleweed/lib/python3.8/site-packages/ansible_collections
Collection        Version
----------------- -------
community.general 3.6.0  

# /home/myhomedir/Documents/Setup/init_tumbleweed/lib64/python3.8/site-packages/ansible_collections
Collection        Version
----------------- -------
community.general 3.6.0

Configuration

$ ansible-config dump --only-changed

OS / Environment

openSUSE Tumbleweed

Steps to Reproduce

- name: Install packages
  zypper:
    name: wireshark

Expected Results

I would expect the wireshark package to be installed along with the recommended packages including wireshark-ui-qt which is the default behaviour of zypper.

Actual Results

The default zypper behaviour of installing recommended packages is overriden by the community.general.zypper package which has disable_recommends: yes as its default value. That breaks package maintainers and package users expectations. If people want to just capture packets, they usually use TCPdump. Wireshark is valued for its GUI interface for capture inspection. However with recommended packages disabled when a user installs the wireshark package they do not get the wireshark-ui-qt package and so are missing the wireshark GUI.

This is obviously not limited to the wireshark package, that is just an example.

Code of Conduct

ansibullbot commented 2 years ago

Files identified in the description:

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

ansibullbot commented 2 years ago

cc @AnderEnder @alxgu @andytom @commel @dcermak @evrardjp @lrupp @sealor @toabctl click here for bot help

felixfontein commented 2 years ago

Changing the default breaks backwards compatibility, so this is somewhat more complicated.

If this option is set to false/no, then the module will not pass the --no-recommends parameter. What will happen then? Is there a default for the package manager that can be configured and will apply if that option is not passed? Is there an explicit option for "install recommended, and ignore what is configured for the system"?

je2555 commented 2 years ago

@felixfontein

Here's the section from the zypper manpage:

           --recommends
               Install also recommended packages in addition to the required ones. The default behavior is determined by [zypp.conf:solver.onlyRequires].

           --no-recommends
               Do not install recommended packages, but only required ones. The default behavior is determined by [zypp.conf:solver.onlyRequires].

The conf parameter referred to is solver.onlyRequires in /etc/zypp/zypp.conf. That file has the following configuration by default:

## Whether only required packages are installed.
##
## Recommended packages, will not be regarded.
##
## Valid values: boolean
## Default value: false
##
# solver.onlyRequires = false

So omitting the --no-recommends parameter will use the default zypper recommends configuration which is to install recommended packages.

As noted above, the --recommends parameter can be used to explicitly require recommended packages.

I did not realise that the module doesn't explicitly set the recommended packages behaviour but simply appends --no-recommends - so it's assuming you're using the default configuration and if you're not it would actually have undefined behaviour?

felixfontein commented 2 years ago

[...] The conf parameter referred to is solver.onlyRequires in /etc/zypp/zypp.conf.

Thanks!

That file has the following configuration by default:

## Whether only required packages are installed.
##
## Recommended packages, will not be regarded.
##
## Valid values: boolean
## Default value: false
##
# solver.onlyRequires = false

So omitting the --no-recommends parameter will use the default zypper recommends configuration which is to install recommended packages.

... if the default config hasn't been changed.

As noted above, the --recommends parameter can be used to explicitly require recommended packages.

I did not realise that the module doesn't explicitly set the recommended packages behaviour but simply appends --no-recommends - so it's assuming you're using the default configuration and if you're not it would actually have undefined behaviour?

It's not undefined, but not exactly what is promised in the documentation either.

I think the optimal behavior should be:

  1. If the option is set to true, it should explicitly pass --recommends.
  2. If the option is set to false, it should explicitly pass --no-recommends.
  3. If the option is not specified (or is none), it should not pass either of these two options.

What I'm not really sure is how should we reach this. Both fixing the obvious bug - pass --recommends if the option is true -, and changing the default to "don't pass anything", are breaking changes.

CC @wombelix who's also interested in this module.

je2555 commented 2 years ago

I think the optimal behavior should be:

1. If the option is set to `true`, it should explicitly pass `--recommends`.

2. If the option is set to `false`, it should explicitly pass `--no-recommends`.

3. If the option is not specified (or is `none`), it should not pass either of these two options.

Sounds perfect to me and it mirrors the apt module exactly.

wombelix commented 2 years ago

I think the optimal behavior should be:

1. If the option is set to `true`, it should explicitly pass `--recommends`.

2. If the option is set to `false`, it should explicitly pass `--no-recommends`.

3. If the option is not specified (or is `none`), it should not pass either of these two options.

Shouldn't it be the other way around?

The option is called disable_recommends, so True should pass the argument --no-recommends (what it already does) and False should use --recommends instead. To align with builtin.apt, the zypper module should default to False instead True. Point 3 makes sense to just leave it to the OS in case of None, which ships with solver.onlyRequires = false.

Compared to the option install_recommends from builtin.apt:

Corresponds to the --no-install-recommends option for apt. yes installs recommended packages. no does not install recommended packages. By default, Ansible will use the same defaults as the operating system. Suggested packages are never installed.

felixfontein commented 2 years ago

Shouldn't it be the other way around?

You are right.

To align with builtin.apt, the zypper module should default to False instead True.

The default for ansible.builtin.apt's option is "use whatever is configured in the system", and not one specific choice. So there should be no default here to align with that module.

wombelix commented 2 years ago

The default for ansible.builtin.apt's option is "use whatever is configured in the system", and not one specific choice. So there should be no default here to align with that module.

You are right, was just reading the description, didn't checked the code. So the zypper default should be None to leave it up to the OS config then.

How can we introduce such a change from a process perspective? Even though it might be a breaking change, it's actually what I as User would expect from the module, so we could also argue that it's a bugfix?

Ajpantuso commented 2 years ago

How can we introduce such a change from a process perspective? Even though it might be a breaking change, it's actually what I as User would expect from the module, so we could also argue that it's a bugfix?

I would think of it this way. If you were an existing user who suddenly found that recommended packages were being installed in addition to the package you specified because this "fix" had not only been introduced to the current release, but also backported to prior releases you'd probably be even more confused.

My 2 cents is to add a new option, maybe install_recommended (The negative condition of disable_recommends is confusing), which implements the suggested functionality and overrides disable_recommends when specified. Then deprecate disable_recommends as per the usual timeline.

felixfontein commented 2 years ago

Adding a new option is probably the best approach, as I can't think of a good way to change the old one without introducing a new option. Unfortunately also adding a new install_recommended option has a problem: since it's a three-valued option, there's no way to distinguish "user explicitly set it to None" and "user didn't specify it".

So maybe best is to add a new option, disable_recommends_behavior, with default value compatibility, and other value sane (bikeshedding needed :) ). Then (in a later release) we can deprecate the default of this new option and make it switch to the new default in the future. Until then, users can set it explicitly to compatibility if they prefer the old usage, or better explicitly set it to sane (and make sure that the value of disable_recommends does what they want). And eventually disable_recommends will behave correctly by default.

ansibullbot commented 1 year ago

Files identified in the description:

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