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

ini_file: Incorrect handling when multiple options with the same name exist #273

Closed
C0rn3j opened this issue May 4, 2020 · 9 comments · Fixed by #3033
Closed

ini_file: Incorrect handling when multiple options with the same name exist #273

C0rn3j opened this issue May 4, 2020 · 9 comments · Fixed by #3033
Labels
bug This issue/PR relates to a bug module module

Comments

@C0rn3j
Copy link
Contributor

C0rn3j commented May 4, 2020

SUMMARY

Option handling is inconsistent when there's multiple of them and there is seemingly no way to keep multiple options.

ISSUE TYPE
  • Bug Report
COMPONENT NAME

ini_file

ANSIBLE VERSION
ansible 2.9.7
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/root/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.8/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.8.2 (default, Apr  8 2020, 14:31:25) [GCC 9.3.0]
CONFIGURATION
N/A
OS / ENVIRONMENT

Arch Linux

STEPS TO REPRODUCE

Play:

- name: Ensure "Include=/etc/pacman.d/mirrorlist is in section "[multilib]" in specified file
  ini_file:
    path: /etc/pacman.conf
    section: multilib
    option: Include
    value: /etc/pacman.d/mirrorlist

How the file /etc/pacman.conf looks before the play:

[multilib]
Include = /etc/pacman.d/pacserve
Include = /etc/pacman.d/mirrorlist

After:

[multilib]
Include = /etc/pacman.d/mirrorlist

I'd like the play not to clobber existing Options, but there does not seem to be a way to do that.

Additionally, if the file looks like this, with the matching entry first, nothing is changed, which is inconsistent.

[multilib]
Include = /etc/pacman.d/mirrorlist
Include = /etc/pacman.d/pacserve
@ansibullbot
Copy link
Collaborator

Files identified in the description:

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

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added affects_2.10 bug This issue/PR relates to a bug module module labels May 4, 2020
@felixfontein
Copy link
Collaborator

There is no "The INI format", there are many different interpretations out there what valid INI formats should be. In particular, there is no consensus whether multiple options with the same name can exist or not (see https://en.wikipedia.org/wiki/INI_file#Duplicate_names). So this is rather a feature request than a bug, because the module documents nowhere that it supports duplicate names.

Also, as the module interface is right now (with value being string and not list), I would expect the module to always end up in the state where the option appears exactly once in the INI file, if it would support duplicate entries.

BTW, there used to be a PR adding this feature: ansible/ansible#19536 Unfortunately it does not mention the interface and all changes are gone (maybe accidentally rebased to nil).

@C0rn3j
Copy link
Contributor Author

C0rn3j commented May 13, 2020

BTW, there used to be a PR adding this feature: ansible/ansible#19536 Unfortunately it does not mention the interface and all changes are gone (maybe accidentally rebased to nil).

I've contacted author of the PR and they don't have the fork anymore.

Also tried unsuccessfully reaching @StoneISStephan about it, so I guess the PR is really gone.

@rosowiecki
Copy link
Contributor

rosowiecki commented May 24, 2020

Loks like whole StonelSStephan/ansible repository is gone as well. Last comments suggest that there wer some unseolved problems with that PR anyway.

@rosowiecki
Copy link
Contributor

rosowiecki commented May 24, 2020

BTW: how about multpiple sections with same name? ini_file module doesn't care about them at all, all changes seem to be applied only to first [section] appeareance. Any suggestions how to handle them properly? (combine together in one section in output OR leave multiple sections but treat all the options as if they were in one section OR fail with error?) Wikipedia says that 'Interpretation of multiple section declarations with the same name also varies' as well, so all possibilities may apply :/

@felixfontein
Copy link
Collaborator

Hmm, multiple sections with the same name sound... not that simple to handle, indeed.

For multiple values (in one section), a good interface would probably be having two options (mainly for backwards compatibility): value of type str, and values of type list[str], which are mutually exclusive, and value: v is equivalent to values: [v].

For multiple sections, it probably makes sense to provide an index so that the i-th section of that name is meant (with failure if a section with that index does not exist, or some configurability of what to do when it doesn't exist, like fail, use the last existing, or add a new one).

@rosowiecki
Copy link
Contributor

For multiple values (in one section), a good interface would probably be having two options (mainly for backwards compatibility): value of type str, and values of type list[str], which are mutually exclusive, and value: v is equivalent to values: [v].

This would require repeating all values when you want to add only one more Include setting to section... I propose additonaly: state: add/remove with following semantics:

  • absent: remove option = lines (all when value[s] == None or only option = value[s])
  • present: keep/insert option = value[s] lines and remove all others
  • add/remove: just ensure that specified settings are added/removed but do not touch others

About multiple section with some name: they aren't very popular anyway, so maybe just have ini_file module operate on first [section] instance only?

@amg1127
Copy link

amg1127 commented Jun 15, 2021

Hi there,

I wanted to comment that systemd configuration frequently uses multiple options with a same name on their configuration files. Without support for multiple options with the same name in ini_file module, idempotent systemd configuration can be achieved with modules such as template or blockinfile, however those alternatives may unwillingly remove settings unmanaged by Ansible.

Support for multiple same-name options in ini_file module would be much appreciated.

Example case 1

An administrator can define startup ordering by specifying multiple After= and Before= arguments, as documented in systemd.unit(5) man page.

# /etc/systemd/system/foo.service
[Unit]
After=network.target
After=network-online.target
Before=httpd.service
Before=mariadb.service

Example case 2

Environment variables for processes started via systemd can be defined by adding one or more Environment= options in [Service] section, as documented in systemd.exec(5) man page.

# /etc/systemd/system/foo.service
[Service]
Environment=PIDFILE=/run/foo.pid
Environment=CACHEFILE=/var/lib/foo/cache.dat
Environment=APITOKEN=55166003-10db-4919-92aa-965b31093151

Example case 3

An administrator willing to override a package-maintained configuration parameter usually create an override file, sets a blank value in order to clear the default values and, then, sets desired values after the blank one. Details are documented in systemd.unit(5) man page.

# /etc/systemd/system/foo.service.d/override.conf
[Unit]
ConditionPathExists=
ConditionPathExists=/this/file/must/exist
ConditionPathExists=!/this/file/must/not/exist

ziegenberg added a commit to ziegenberg/community.general that referenced this issue Aug 9, 2021
 - restructure tests
 - fix error message call: fail_json() takes 1 positional argument but 2 were given
ziegenberg added a commit to ziegenberg/community.general that referenced this issue Aug 9, 2021
  - add module option 'exclusive' (boolean) for the abbility to add
    single option=value entries without overwriting existing options
    with the same name but different values
  - add abbility to define multiple options with the same name but
    different values
felixfontein pushed a commit that referenced this issue Aug 15, 2021
* ini_file - prepare for fixing #273
 - restructure tests
 - fix error message call: fail_json() takes 1 positional argument but 2 were given

* ini_file - multiple values for one option (#273)
  - add module option 'exclusive' (boolean) for the abbility to add
    single option=value entries without overwriting existing options
    with the same name but different values
  - add abbility to define multiple options with the same name but
    different values

* ini_file - add more tests for ini_file

* ini_file - fix sanity tests

* apply suggested changes:

- rename 03-regressions.yml to 03-encoding.yml
- fix typos
- fix documentation

* apply suggested changes:

- test errors also for result is failed

* apply suggested changes:

- make state=absent also work with module option exclusive
- add more tests for state=absent and module option exclusive

* fix sanity test:

- 02-values.yml:251:9: hyphens: too many spaces after hyphen

* apply proposed changes

* apply proposed changes from review
- adjust version_added to 3.6.0
- small syntax change in changelog fragment
patchback bot pushed a commit that referenced this issue Aug 15, 2021
* ini_file - prepare for fixing #273
 - restructure tests
 - fix error message call: fail_json() takes 1 positional argument but 2 were given

* ini_file - multiple values for one option (#273)
  - add module option 'exclusive' (boolean) for the abbility to add
    single option=value entries without overwriting existing options
    with the same name but different values
  - add abbility to define multiple options with the same name but
    different values

* ini_file - add more tests for ini_file

* ini_file - fix sanity tests

* apply suggested changes:

- rename 03-regressions.yml to 03-encoding.yml
- fix typos
- fix documentation

* apply suggested changes:

- test errors also for result is failed

* apply suggested changes:

- make state=absent also work with module option exclusive
- add more tests for state=absent and module option exclusive

* fix sanity test:

- 02-values.yml:251:9: hyphens: too many spaces after hyphen

* apply proposed changes

* apply proposed changes from review
- adjust version_added to 3.6.0
- small syntax change in changelog fragment

(cherry picked from commit 25267b8)
felixfontein pushed a commit that referenced this issue Aug 15, 2021
)

* ini_file - prepare for fixing #273
 - restructure tests
 - fix error message call: fail_json() takes 1 positional argument but 2 were given

* ini_file - multiple values for one option (#273)
  - add module option 'exclusive' (boolean) for the abbility to add
    single option=value entries without overwriting existing options
    with the same name but different values
  - add abbility to define multiple options with the same name but
    different values

* ini_file - add more tests for ini_file

* ini_file - fix sanity tests

* apply suggested changes:

- rename 03-regressions.yml to 03-encoding.yml
- fix typos
- fix documentation

* apply suggested changes:

- test errors also for result is failed

* apply suggested changes:

- make state=absent also work with module option exclusive
- add more tests for state=absent and module option exclusive

* fix sanity test:

- 02-values.yml:251:9: hyphens: too many spaces after hyphen

* apply proposed changes

* apply proposed changes from review
- adjust version_added to 3.6.0
- small syntax change in changelog fragment

(cherry picked from commit 25267b8)

Co-authored-by: Daniel Ziegenberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug module module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants