-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.options_present adds unnecessary whitespaces #33669
Comments
@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? |
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. |
How about a |
This is annoying. It partially breaks usage of this module for me... |
@lorengordon #31210 doesn't actually address this issue. The key piece of code in that PR related to this issue is the following line:
So if someone adds
This is as opposed to the default which is:
If someone adds something other than space, they will still also get extra whitespace. eg. if the separator is set to '->' they will get:
Now we could instead just change that one line to look like this:
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:
If an application parser required The only clean option I see is to add another argument the user must specify to avoid the whitespace. eg. having a default of 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.
I am working to address that here: #50613 |
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) |
@Poil We have Such an option would be incompatible with |
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 Given that there is not actual If anything, a principle of least surprise should be followed -> BTW, Puppet has done this the right way for years :^) |
As it stands, I just tried to managing Could we take the 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:
I.e. the empty |
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. |
Files under 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! |
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 |
@galet If I understand correctly, what you are proposing is:
as in three characters - As long as with:
I don't get spaces around it, then I'm all for it! Great idea! :^) Thanks! |
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. |
@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 Unfortunately, shell style variable assignments (no spaces around "=" allowed) are very common for a number of tools or applications. |
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 But maybe this would be too "automatic" for some people's taste? It's also more work to implement. |
A real life
Now, I'd like to manage the former and the latter. Can |
IMVHO, the
By all means, make the default to be backward compatible, but do break backward compatibility if it is the only way to move forward. |
But
It's actually the default on Debian. It has been for as long as I can remember. |
So, still no ways to remove unnecessary spaces from ini separators?
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 |
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. |
Any progress on this? |
Any updates here? Was the PR ever reviewed? |
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 |
Looks like the fix is here: #60081 |
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.
The following state file...
... produces the following content in
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 #60081 as suggested by twangboy on Aug 21, 2023 |
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. |
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
results in
In 2016.3 the following
results in
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.
The text was updated successfully, but these errors were encountered: