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

Add option to allow SimpleConfig to strip trailing whitespace #2402

Closed
wants to merge 5 commits into from

Conversation

miah
Copy link
Contributor

@miah miah commented Dec 15, 2017

This PR Updates SimpleConfig to allow users to strip trailing whitespace from parsed values. The :strip_values option is defaulted to false, users must explicitly enable it.

Note the whitespace after www.inspec.test:

SimpleConfig.new('ServerAlias www.inspec.test ', assignment_regex: /^\s*(\S+)\s+(.*)\s*$/, multiple_values: true, strip_values: true)

Will result in params including { 'ServerAlias' => ['www.inspec.test'] }

This PR also updates the apache_conf resource to enable :strip_values by default.

Because Apache allows you to pass multiple values, and define the setting more than once you can still end up with parameters like this:

# apache2.conf
ServerAlias io.inspec.test inspec.test
ServerAlias www.inspec.test

{ 'ServerAlias' => ['io.inspec.test inspec.test', 'www.inspec.test'] }

But they will no longer include trailing whitespace =)

Fixes #1241

Signed-off-by: Miah Johnson <miah@chia-pet.org>
Signed-off-by: Miah Johnson <miah@chia-pet.org>
whitespace.

Signed-off-by: Miah Johnson <miah@chia-pet.org>
Signed-off-by: Miah Johnson <miah@chia-pet.org>
Signed-off-by: Miah Johnson <miah@chia-pet.org>
@miah miah requested a review from a team as a code owner December 15, 2017 20:23
@adamleff
Copy link
Contributor

So, I'm a bit on-the-fence on this change. Part of me thinks that this feature shouldn't be necessary; the assignment_regex should just be written properly to not be greedy and pull in spaces. Am I crazy for thinking that?

Also, I think I'm going to engage SUPER PEDANT MODE and suggest that (if we proceed with this change) the parameter be named strip_whitespace to make it clearer to the user what the outcome will be.

Let's chat about this sometime tomorrow if you don't mind!

@miah
Copy link
Contributor Author

miah commented Dec 21, 2017

I was also on the fence with this change. I worked with @jerryaldrichiii on a regex solution to this. New PR incoming =)

@miah miah closed this Dec 21, 2017
@adamleff
Copy link
Contributor

@miah thank you for going back and taking a second look at it after the initial feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants