ini.options_present adds unnecessary whitespaces

[giannello]

Description of Issue/Question

With the 2016.3.0 release, ini.options_present adds unnecessary (sometimes breaking) whitespaces around the key/value separator

Setup

On 2015.8

test:
  ini.options_present:
    - name: /tmp/test
    - sections:
        DEFAULT_IMPLICIT:
          LANG: 'en_US.UTF-8'
          LC_ALL: 'en_US.UTF-8'

results in

LANG=en_US.UTF-8a
LC_ALL=en_US.UTF-8a

In 2016.3 the following

test:
  ini.options_present:
    - name: /tmp/test
    - sections:
        LANG: 'en_US.UTF-8'
        LC_ALL: 'en_US.UTF-8'

results in

LANG = en_US.UTF-8a
LC_ALL = en_US.UTF-8a

In our specific case, we use ini_manage to handle some files that are sourced by the shell (/etc/default/locales). Adding whitespaces around the "=" breaks the environment.

Also, even if the file already contains the right values and there's no whitespaces around the separator, whitespaces are added and the return message reports that no changes have been applied to the file.

[Ch3LL]

@giannello I am able to replicate this. Looks like this is related to #21592 and #26359

ping @Akilesh1597 looks like this was done on purpose, due ot another user wanting the space in their configuration file. Do you see a solution to this issue? Possibly adding a switch to either add/remove whitespace?

[Akilesh1597]

The space was added for the purpose of readability of ini file. It would definitely break a file with some evn variable. I'll see if I can add a flag somewhere.

[lorengordon]

How about a separator or assignment param? It can default to '=' and if you want spaces, just set it to ' = '.

[dstensnes]

This is annoying. It partially breaks usage of this module for me... Can we please have a fix for this soon?

[lorengordon]

Can probably be closed, as #31210 was merged. @Ch3LL?

[boltronics]

@lorengordon #31210 doesn't actually address this issue. The key piece of code in that PR related to this issue is the following line:

' {0} '.format(self.sep) if self.sep != ' ' else self.sep

So if someone adds separator: ' ' to their state file, they can get an ini file looking like this:

[example_1]
some_option on
another_option off

This is as opposed to the default which is:

[example_2]
some_option = on
another_option = off

If someone adds something other than space, they will still also get extra whitespace. eg. if the separator is set to '->' they will get:

[example_3]
some_option -> on
another_option -> off

Now we could instead just change that one line to look like this:

'{0}'.format(self.sep) if self.sep != ' ' else self.sep

This would result in the first example looking the same and the second example doing what @giannello is trying to achieve, but then the third example would look like this:

[example_3]
some_option->on
another_option->off

If an application parser required -> to be surrounded by whitespace, the code change would break that application.

The only clean option I see is to add another argument the user must specify to avoid the whitespace. eg. having a default of trim_whitespace=False which can be changed if the user desires. I can easily implement that... but I question if that's a good thing for the project to have? Are there any use cases for an ini-like file format that cannot have whitespace between the separator character? I do think using this module to generate shell scripts just isn't what it's meant for, and most probably file.replace is a better fit for that sort of task.

Having said that, it's certainly not my call. I'll wait to hear what one of the Salt devs such as @Ch3LL think should be done.

Also, even if the file already contains the right values and there's no whitespaces around the separator, whitespaces are added and the return message reports that no changes have been applied to the file.

I am working to address that here: https://github.com/saltstack/salt/pull/50613

[Poil]

I personally don't like file.replace because if a new version of a package add new options you will not be advertised, (yes I prefer to have something broken)

[boltronics]

@Poil We have ignore_if_missing to control what to do if the file is missing. Perhaps it would make more sense to add something similar - an option there named something like ignore_if_no_matches that does nothing unless set to False, whereby it errors out?

Such an option would be incompatible with append_if_not_found/prepend_if_not_found, but I think would otherwise be relatively straightforward. Would that meet your requirements?

[rjc]

I've been hit by this issues today, myself.

The easiest way out that I can see is, adjusting the documentation and mentioning the extra (superfluous) spaces around the separator but, at the same time, provide an option to have the separator as '=', without spaces.

Given that there is not actual INI standard, most programs refer to this as INI-style file format, and some are sensitive to spaces around the separator.

If anything, a principle of least surprise should be followed -> '=' used, foo=bar expected; ' = ' used, bar = baz expected; '= ' used, opt= val expected, etc.

BTW, Puppet has done this the right way for years :^)

[rjc]

As it stands, ini currently only works with a specific type of INI files, i.e.: ones with sections in square brackets ([]), etc.

I just tried to managing /etc/default/exim4 on Debian/Ubuntu and, after the '=' -> ' = ' change, the exim4 service would not start.

Could we take the P4 label off, please? These aren't corner cases any more.

While we're at it, is there any way to make section-less INI-style files easier to manage? Currently, I have to do this:

/etc/default/exim4:
  ini.options_present:
    - sections:
          QUEUERUNNER: "'queueonly'"

I.e. the empty -sections: and spacing before the option: value found by trial and error.

[drel4o]

Hello,

+1 for no spaces around the separator.

Our case is altering apache2 envvars on ubuntu. There is just no other salt module more convenient than this one. D

[boltronics]

Files under /etc/default/ are all expected to be shell scripts that are sourced. Some of those files even have shell functions in them (eg. the one provided by the aufs package)! As for apache vhosts, there is an entire module dedicated to that use-case built in, and I don't see them as even being close to the traditional ini format, which has no concept of things like closing tags.

My take is that both of these fall well outside the scope of ini file management. I've personally never seen an actual ini file that has an issue with whitespace padded around the delimiter, although I'm happy to be corrected if examples can be provided. As such, while I don't have anything against supporting the requested changes discussed here, I would view them both as feature requests, and not bugs per se. I'm a bit surprised the High Severity and Bug tags still remain attached.

One thing that I find very surprising about Salt's ini_manage module is that it wasn't written to use Python's built-in ConfigParser. Going through the commit history, it started out as using a bunch of regular expressions! I think we can do better, and probably wouldn't have seen the subtle change in whitespace behaviour were we letting the built-in library handle the bulk of the work.

I can only theorise that perhaps the ConfigParser included in Python 2.7 wasn't considered flexible enough, although with less than a year now before 2.7 goes EOL, that shouldn't be much of a worry at this point!

[galet]

There is a number of cases where the spaces are problematic and there is no other elegant alternative in Salt. It is very frustrating. Wouldn't a simple change of the default delimiter from '=' to ' = ' and corresponding formatting line solve the problem without significantly breaking backward compatibility?

[rjc]

@galet If I understand correctly, what you are proposing is:

- separator: ' = '

as in three characters - = and a space on either side? With additional spaces removed from formattig that would work. And indeed, no backward compatibility issues! :^)

As long as with:

- separator: '=>'

I don't get spaces around it, then I'm all for it! Great idea! :^)

Thanks!

[boltronics]

Wouldn't a simple change of the default delimiter from '=' to ' = ' and corresponding formatting line solve the problem without significantly breaking backward compatibility?

Unfortunately not. See my comment from Nov 2018 above, where I mention the problem with this and also proposed a possible solution that doesn't break backwards compatibility.

[galet]

@boltronics I agree with you, that's why I used "significantly" in that sentence:) Your proposed solution by adding additional parameter to trim the whitespace is perfectly fine with me.

One could argue example3 is far less common so those usages would be broken and would require changes by users (changing their settings from "->" to " -> "). The question is whether simplicity (avoiding additional parameters) is more valuable than braking the backward compatibility for some users. I'm not in a position to express a preference here.

Unfortunately, shell style variable assignments (no spaces around "=" allowed) are very common for a number of tools or applications. file.maged or file.replace simply cannot be used (e.g. file not completely managed by salt and updated by the application too) or are very cumbersome (file.replace). Therefore, any solution to this issue would be very welcomed.

[boltronics]

Hmm... if using this for shell scripts, I would have thought a good regex with file.replace would be more reliable honestly, but I'd be interested to read about any problematic examples to get a better understanding.

I've looked around my system, and the majority of ini files I have have use " = ". Only one doesn't, but looks like it permits whitespace all the same.

On Windows it's the opposite, where there are some examples of " = " but the majority use "=".

In my personal opinion, we should scan the existing ini file and see what is being used for the first option found (or the option being edited if it already exists) and default to using that, with an option to turn off any auto-detection with a trim_whitespace argument which can either be set to True or False as preferred. If we are working on a brand new ini file with no pre-existing options, we would default to " = " on all hosts except Windows, where we default to "=", and we would document this behaviour.

But maybe this would be too "automatic" for some people's taste? It's also more work to implement.

[rjc]

A real life my.cnf example:

[ndbd]
connect-string=ndb_mgmd.mysql.com

[ndb_mgm]
connect-string=ndb_mgmd.mysql.com

Now, I'd like to manage the former and the latter. Can file.replace do that easily?

[rjc]

IMVHO, the separator should always be verbatim, without spaces around it, i.e.:

' ='
'= '
' = '
'='
'=>'
[...]

By all means, make the default to be backward compatible, but do break backward compatibility if it is the only way to move forward.

[boltronics]

A real life my.cnf example:

But my.cnf does accept whitespace around the = separator. :smiley:

$ grep -v -e '^#.*$' -e '^$' /etc/mysql/my.cnf
!includedir /etc/mysql/conf.d/
!includedir /etc/mysql/mysql.conf.d/
$ grep -v -e '^#.*$' -e '^$' /etc/mysql/mysql.conf.d/mysqld.cnf 
[mysqld_safe]
socket      = /var/run/mysqld/mysqld.sock
nice        = 0
[mysqld]
user        = mysql
pid-file    = /var/run/mysqld/mysqld.pid
socket      = /var/run/mysqld/mysqld.sock
port        = 3306
basedir     = /usr
datadir     = /var/lib/mysql
tmpdir      = /tmp
lc-messages-dir = /usr/share/mysql
skip-external-locking
bind-address        = 127.0.0.1
key_buffer_size     = 16M
max_allowed_packet  = 16M
thread_stack        = 192K
thread_cache_size       = 8
myisam-recover         = BACKUP
query_cache_limit   = 1M
query_cache_size        = 16M
log_error = /var/log/mysql/error.log
expire_logs_days    = 10
max_binlog_size   = 100M
$ 

It's actually the default on Debian. It has been for as long as I can remember.

[rjc]

This was just an example to illustrate the fact that regex isn't always the answer, not an example "whitespace around the separator or not" ;^)

[boltronics]

The way I see it, applications will either support the usual ini format that permits whitespace around the option separator, or people are abusing the module to make it work for shell scripts and the like, where whitespace around the separator isn't permitted, as per the OP.

That's why I wanted a real example. What you posted wouldn't be meaningful for simple shell scripts (where presumably the section would be specified as a comment) because it would just be updating the same variable twice with two different values. In the case of a shell script, file.replace would be quite painless to use for such purposes (eg. /etc/default/ file manipulation).

I'm certainly not against supporting additional functionality, but maybe a separate module would be more appropriate? In which case, the ini module and whatever else could even share some of the same underlying code if appropriate. The Zen of Python states "There should be one-- and preferably only one --obvious way to do it.", and I don't think that using the ini module to manage a shell script fits that criteria.

As I think I stated earlier, I personally believe this module should be completely re-written to use Python's native configparser library to improve its reliability and extend its feature set. I believe configparser can easily do everything required - but (at a glance) perhaps not for whitespace around the separator without doing some kind of pre-processing of the file first. I could be wrong.

[rjc]

The way I see it, applications will either support the usual ini format that permits whitespace around the option separator[...]

From experience, there is no usual INI format and the crucial word there is permits.

The module works for a narrow subset of INI-style files. And it clearly works for you, I get it. But for a lot of us, it doesn't and we would like it fixed as, currently, it is broken. Like I've mentioned above, Puppet had a working module for years:

The inifile module is used to:

Support comments starting with either '#' or ';'. Support either whitespace or no whitespace around '='. Add any missing sections to the INI file.

It does not manipulate your file any more than it needs to. In most cases, it doesn't affect the original whitespace, comments, or ordering. See the common usages below for examples.

I used to use it to manage lightdm.conf and users.conf files which, by default, don't have whitespace around '='.

[boltronics]

Thanks - when I searched my system for .ini files earlier, I forgot that many .conf files are basically also .ini files, such as those used by systemd (which also don't use whitespace by default). Even if not strictly required AFAICT, I get that it would be preferable to keep everything consistent.

I took a moment to take a closer look into how and why the other projects manage this:

It looks like Puppet just defaults to using ' = ', which is what they were already defaulting to when they first added that option (here).

Ansible is similar to what I had originally proposed, with a dedicated no_extra_spaces argument. I find it interesting that, like Puppet, they had the option when adding that feature (here) to allow the user to set a custom assignment format, or to simply have an option to toggle spaces on and off, and chose the later.

Unlike Salt, neither project had a pre-existing separator option which the end user might already be using, when these commits were introduced.

I do like the idea of getting a no_extra_spaces option into Salt as soon as possible (stealing Ansible's name for this too since it's better than "trim_whitespace"), possibly in a point-release since it's arguably a show-stopper. It still offers the same level of flexibility. The alternative, changing the default value of separator, would probably take longer to get upstream due to a change in behaviour.

A major release could incorporate more functionality, such as automatically detecting appropriate whitespace or re-using existing whitespace for value changes, etc. where backwards-compatibility is less of a concern.

[Akilesh1597]

This has been open for too long. I can implement the last comment from @boltronics if there are no more arguments.

[sagetherage]

@Akilesh1597 if you want to open a PR we will try to get it merged in Aluminium release cycle

[MurzNN]

So, still no ways to remove unnecessary spaces from ini separators? To not break old behavior, maybe add separate option to manage spacing rules like:

no_spaces: True

And add spaces to separator variable manually, if this option is false, something like this: https://github.com/saltstack/salt/pull/60081

[sagetherage]

@MurzNN

So, still no ways to remove unnecessary spaces from ini separators? To not break old behavior, maybe add separate option to manage spacing rules like:

no_spaces: True

And add spaces to separator variable manually, if this option is false, something like this: #60081

Good question - I have posted this to our Community Slack workspace https://saltstackcommunity.slack.com/archives/C8832QN4U/p1619474427216100 I will do my best to follow up and I hope this helps.

[MurzNN]

Before this issue is solved, as workaround we can reuse https://docs.saltproject.io/en/latest/ref/states/all/salt.states.augeas.html for manage ini file settings.

[vincent-olivert-riera]

Any progress on this?

[apokryphal]

Any updates here? Was the PR ever reviewed?

[boltronics]

I'm not a project member, but the PR looks good to me (although I haven't tried it). I'm guessing it should have a test case associated with it though, which might be why it was not given a closer look.

I'd also prefer it used no_extra_spaces instead of no_spaces for consistency and clarity, but that's a nitpick.

[twangboy]

Looks like the fix is here: https://github.com/saltstack/salt/pull/60081 Just need to write some simple tests and generate a changelog

[sscotter]

Apologies if this is a newbie error (I've only been using Salt a matter of weeks) but I think the white space around seperator "bug" is still present.

I'm running the following version across a pair of Debian and Ubuntu test hosts.

salt-minion --version
salt-minion 3007.1 (Chlorine)

The following state file...

file_etc_systemd_timesyncd.conf:
  ini.options_present:
    - name: /etc/systemd/timesyncd.conf.d/local.conf
    - separator: '='
    - sections:
        Time:
          NTP: ntp1.domain.local

... produces the following content in /etc/systemd/timesyncd.conf.d/local.conf ...

[Time]
NTP = ntp1.example.local

While the output doesn't break functionality in this case, I personally think it doesn't read well and more importantly seems to be at odds to the fix apparently remediated by https://github.com/saltstack/salt/pull/60081 as suggested by twangboy on Aug 21, 2023

[twangboy]

The above PR should address this issue. Thanks, @MurzNN for the idea on how to fix it. It was a little more involved than that, but I think I got it.