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

Modify linux regular expression to handle process names with spaces #2117

Merged
merged 2 commits into from
Sep 5, 2017

Conversation

ChadScott
Copy link
Contributor

Fixes #1497

Tested on Ubuntu 14.04, Ubuntu 16.04, and CentOS 7.3

@ChadScott ChadScott requested a review from a team as a code owner September 1, 2017 22:14
@adamleff
Copy link
Contributor

adamleff commented Sep 1, 2017

Hi, @ChadScott! Thank you for your first PR to InSpec!

This is a great change, thank you. Can you please enhance the unit tests for this resource so that we make sure we don't have a future regression for this use case?

You'll need to add a sample line to the mock call here: https://github.com/chef/inspec/blob/master/test/unit/mock/cmd/ps-axoZ

... and then modify the tests accordingly here:

https://github.com/chef/inspec/blob/master/test/unit/resources/processes_test.rb

Let me know if any of this is unclear or if you need any assistance. Thank you again!

@adamleff adamleff added the Type: Bug Feature not working as expected label Sep 1, 2017
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also @ChadScott it looks like you've got some failing unit tests for the existing behavior (lots of extra untrimmed space, for example). Please take a look at the Travis job and you'll see what I mean.

@ChadScott
Copy link
Contributor Author

@adamleff the tests are failing in one case and it looks like it's because the .+ is greedy instead of the \s+... I'm looking into how to better optimize that. Stand by.

Signed-off-by: Chad Scott <[email protected]>
@ChadScott
Copy link
Contributor Author

a3b70ab addresses your concerns, I believe.

@ChadScott
Copy link
Contributor Author

Well, now it appears to be failing on my CentOS instance.

Let me hack at it a bit more.

@ChadScott
Copy link
Contributor Author

Disregard my local failure. It was the wrong gem (wasn't using my local copy)... I've validated this change against a real-world environment on Ubuntu 14.04, Ubuntu 16.04, and CentOS 7.3.

Should be okay to merge.

Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid to me! Thanks, @ChadScott!

@adamleff adamleff requested a review from a team September 5, 2017 12:28
Copy link
Contributor

@arlimus arlimus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, thank you so much @ChadScott !!
kudos @adamleff for your review

@arlimus arlimus merged commit 09b1451 into inspec:master Sep 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants