ini_file: try using inactive option before creating a new one #6575

njutn95 · 2023-05-26T21:05:39Z

SUMMARY

Not sure if this is a bug or not, but it definitely impacts specific scenarios.

Code change updates the behavior of attempting to utilize already existing options that are commented out (i.e. inactive) by making them active with the new value, before creating a new option which leads to having both inactive and active options.

It is considered a bug fix because of the default php-fpm.conf configuration which looks like this:

[global]
...
; Time limit for child processes to wait for a reaction on signals from master.
; Available units: s(econds), m(inutes), h(ours), or d(ays)
; Default Unit: seconds
; Default Value: 0
;process_control_timeout = 0

; ...

include=/etc/php/8.1/fpm/pool.d/*.conf

where the pool.d/*.conf is introducing a new section called [www] after the include.

With the original code, the following task:

ini_file:
    path: /etc/php/8.1/fpm/php-fpm.conf
    section: global
    option: process_control_timeout
    value: '30'

would produce the following output

[global]
...
; Time limit for child processes to wait for a reaction on signals from master.
; Available units: s(econds), m(inutes), h(ours), or d(ays)
; Default Unit: seconds
; Default Value: 0
;process_control_timeout = 0

; ...

include=/etc/php/8.1/fpm/pool.d/*.conf
process_control_timeout = 30

leading to process_control_timeout being applied to [www] section instead of [general], whereas the updated code will produce the following:

[global]
...
; Time limit for child processes to wait for a reaction on signals from master.
; Available units: s(econds), m(inutes), h(ours), or d(ays)
; Default Unit: seconds
; Default Value: 0
process_control_timeout = 30

; ...

include=/etc/php/8.1/fpm/pool.d/*.conf

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

ini_file

ADDITIONAL INFORMATION

…reating a new option entry Add changelog fragment

ansibullbot · 2023-05-26T21:10:25Z

cc @jpmens @noseka1
click here for bot help

felixfontein

I'm not really sure this is a bug, or more a new feature. The module doesn't document that it should activate inactive options whenever possible, but on the other hand its strange that it did so in some cases, but not in others.

On the other hand, whether using "inactive" options is a good idea is something that really depends a lot on the context. In some cases, this might destroy and invalidate documentation in the comments of an INI file, while in other cases (like yours) this is a good thing.

I guess this is another case where the various different syntaxes and ways INI files are used cause a lot of problems and confusion...

changelogs/fragments/ini_file-use-inactive-options-when-possible.yml

…le.yml Co-authored-by: Felix Fontein <felix@fontein.de>

njutn95 · 2023-05-29T22:16:08Z

I'm not really sure this is a bug, or more a new feature

The reason I'm considering it a bug is because it was the intended behavior couple of years ago which, after researching a bit, got broken by this commit where it's not clear to me that it was the intended change to the behavior.

But yeah, I agree with you that the way INI config exists in the wild is not always predictable, and although my change is not necessarily a better approach, it feels to me that it's less prone to causing errors than the existing setup (due to the condition given in the example, which is a common condition to be found in the wild based on my experience).

felixfontein · 2023-06-04T20:12:24Z

CC @ziegenberg who originally implemented that feature.

ziegenberg · 2023-06-09T12:29:59Z

With the mentioned commit, I introduced the new option exclusive, which, if set to true, added the ability to add single option=value entries without overwriting existing options with the same name but different values and added the ability to define multiple options with the same name but different values.

~~With exclusive set to false (the default), it should probably use an inactive option and overwrite the value.~~

~~Those changes are in a code branch where exclusive is true.~~

ziegenberg · 2023-06-09T12:38:44Z

Ah, partly disregard my last comment. I mixed up the default values... 🙈

With the mentioned commit, I introduced the new option exclusive, which, if set to false, added the ability to add single option=value entries without overwriting existing options with the same name but different values and added the ability to define multiple options with the same name but different values.

With exclusive set to true (the default), it should probably use an inactive option and overwrite the value.

Those changes are in a code branch where exclusive is true, so I guess this is a bug that needs fixing.

But the tests probably need some more fixing. test-values 23 was changed and test-values 24 is based on the output of test-values 23 so it now no longer tests the same thing.

njutn95 · 2023-06-12T15:38:38Z

@ziegenberg I'm not sure I'm following if this was the intentional change for it to ignore commented key=value and create a new one, or if you agree that this is something that needs fixing 😅

Regarding the test-values-24, I believe that it should be removed (and not replaced, since I don't see the need of testing the same thing, and don't see what it should be replaced with). test-values-23 will just need a name adjusting. I'll handle that once I get a confirmation if this is something that was done intentionally or not.

EDIT: I've updated the tests

felixfontein · 2023-06-15T10:56:24Z

Please note that a new release will happen on Monday. It would be great if the discussion could be resolved and this PR merged by then if everyone's happy with it.

njutn95 · 2023-06-15T13:06:31Z

That sounds good to me, but would appreciate a blessing from @ziegenberg

ziegenberg

I guess, it's ok now.

felixfontein · 2023-06-18T18:31:28Z

Thanks. If nobody objects, I'll merge tomorrow morning for community.general 7.1.0 and 6.6.2.

patchback · 2023-06-19T04:53:37Z

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/f710a10f256da9286b118811a157644ee084576c/pr-6575

Backported as #6726

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* ini_file: make inactive options as active if they exist, instead of creating a new option entry Add changelog fragment * Update changelogs/fragments/ini_file-use-inactive-options-when-possible.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Fix test * Update tests * Fix spelling --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit f710a10)

patchback · 2023-06-19T04:54:08Z

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/f710a10f256da9286b118811a157644ee084576c/pr-6575

Backported as #6728

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

* ini_file: make inactive options as active if they exist, instead of creating a new option entry Add changelog fragment * Update changelogs/fragments/ini_file-use-inactive-options-when-possible.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Fix test * Update tests * Fix spelling --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit f710a10)

felixfontein · 2023-06-19T04:54:51Z

@njutn95 thanks for your contribution!
@ziegenberg thanks for reviewing!

…ption before creating a new one (#6728) ini_file: try using inactive option before creating a new one (#6575) * ini_file: make inactive options as active if they exist, instead of creating a new option entry Add changelog fragment * Update changelogs/fragments/ini_file-use-inactive-options-when-possible.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Fix test * Update tests * Fix spelling --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit f710a10) Co-authored-by: njutn95 <njutn95@yahoo.com>

…ption before creating a new one (#6726) ini_file: try using inactive option before creating a new one (#6575) * ini_file: make inactive options as active if they exist, instead of creating a new option entry Add changelog fragment * Update changelogs/fragments/ini_file-use-inactive-options-when-possible.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Fix test * Update tests * Fix spelling --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit f710a10) Co-authored-by: njutn95 <njutn95@yahoo.com>

rightaway · 2023-10-15T23:53:18Z

This is a big breaking change that should have been introduced with an opt in option.

It is destroying valuable defaults and comments in lots of files.

#7297

felixfontein · 2023-10-16T04:53:34Z

@rightaway did you read my comment #7297 (comment) ?

ini_file: make inactive options as active if they exist, instead of c…

ad00fd1

…reating a new option entry Add changelog fragment

njutn95 force-pushed the bugfix/ini-file/update-inactive-options-if-exist branch from 4dffa7d to ad00fd1 Compare May 26, 2023 21:06

ansibullbot added bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor plugins plugin (any type) needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels May 26, 2023

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-7 Automatically create a backport for the stable-7 branch labels May 29, 2023

felixfontein reviewed May 29, 2023

View reviewed changes

changelogs/fragments/ini_file-use-inactive-options-when-possible.yml Outdated Show resolved Hide resolved

njutn95 and others added 2 commits May 29, 2023 23:51

Update changelogs/fragments/ini_file-use-inactive-options-when-possib…

23e7027

…le.yml Co-authored-by: Felix Fontein <felix@fontein.de>

Fix test

65f6bb6

ansibullbot added integration tests/integration tests tests and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels May 29, 2023

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jun 9, 2023

njutn95 added 2 commits June 15, 2023 16:47

Update tests

4292e4a

Fix spelling

68fe9e1

ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jun 15, 2023

ziegenberg reviewed Jun 16, 2023

View reviewed changes

felixfontein merged commit f710a10 into ansible-collections:main Jun 19, 2023
136 checks passed

patchback bot mentioned this pull request Jun 19, 2023

[PR #6575/f710a10f backport][stable-7] ini_file: try using inactive option before creating a new one #6726

Merged

felixfontein added the backport-6 label Jun 19, 2023

patchback bot mentioned this pull request Jun 19, 2023

[PR #6575/f710a10f backport][stable-6] ini_file: try using inactive option before creating a new one #6728

Merged

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jun 19, 2023

felixfontein mentioned this pull request Sep 20, 2023

Breaking change in ini_file removes comments #7297

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ini_file: try using inactive option before creating a new one #6575

ini_file: try using inactive option before creating a new one #6575

njutn95 commented May 26, 2023

ansibullbot commented May 26, 2023

felixfontein left a comment

njutn95 commented May 29, 2023

felixfontein commented Jun 4, 2023

ziegenberg commented Jun 9, 2023 •

edited

Loading

ziegenberg commented Jun 9, 2023

njutn95 commented Jun 12, 2023 •

edited

Loading

felixfontein commented Jun 15, 2023

njutn95 commented Jun 15, 2023

ziegenberg left a comment

felixfontein commented Jun 18, 2023

patchback bot commented Jun 19, 2023 •

edited

Loading

patchback bot commented Jun 19, 2023 •

edited

Loading

felixfontein commented Jun 19, 2023

rightaway commented Oct 15, 2023

felixfontein commented Oct 16, 2023

ini_file: try using inactive option before creating a new one #6575

ini_file: try using inactive option before creating a new one #6575

Conversation

njutn95 commented May 26, 2023

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented May 26, 2023

felixfontein left a comment

Choose a reason for hiding this comment

njutn95 commented May 29, 2023

felixfontein commented Jun 4, 2023

ziegenberg commented Jun 9, 2023 • edited Loading

ziegenberg commented Jun 9, 2023

njutn95 commented Jun 12, 2023 • edited Loading

felixfontein commented Jun 15, 2023

njutn95 commented Jun 15, 2023

ziegenberg left a comment

Choose a reason for hiding this comment

felixfontein commented Jun 18, 2023

patchback bot commented Jun 19, 2023 • edited Loading

Backport to stable-7: 💚 backport PR created

patchback bot commented Jun 19, 2023 • edited Loading

Backport to stable-6: 💚 backport PR created

felixfontein commented Jun 19, 2023

rightaway commented Oct 15, 2023

felixfontein commented Oct 16, 2023

ziegenberg commented Jun 9, 2023 •

edited

Loading

njutn95 commented Jun 12, 2023 •

edited

Loading

patchback bot commented Jun 19, 2023 •

edited

Loading

patchback bot commented Jun 19, 2023 •

edited

Loading