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

Added option to remove all CRs from input stream #1606

Closed
wants to merge 8 commits into from

Conversation

butitsnotme
Copy link
Contributor

@butitsnotme butitsnotme commented Aug 8, 2016

Required for all PRs:

  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

Added the option removecr to inputs.exec to remove all carriage returns
(CR, ASCII 0x0D, Unicode codepoint \u0D, ^M). The option is boolean and
not enabled if not present in the config file.

To be clear it removes them from the output of the command before it is processed by the parser.

@butitsnotme
Copy link
Contributor Author

I'm not sure why the test failed, it is the TestCombinedOutputTimeout test that is failing. It runs fine on my test machine (Windows) (though many other tests fail because they're expecting services...).

Perhaps the test just need to be re-run? (Or maybe my change affects this, but don't see anything I changed getting called)

@sparrc
Copy link
Contributor

sparrc commented Aug 9, 2016

Why did you make it a configurable option? is there ever a reason someone would want to keep the carriage returns?

@butitsnotme
Copy link
Contributor Author

I can't think of a reason, should I remove the option?

@sparrc
Copy link
Contributor

sparrc commented Aug 10, 2016

yes, please do, but possibly we should be doing this at a lower level in the command running exec functions: https://github.com/influxdata/telegraf/blob/master/internal/internal.go#L168

and it should also only happen if runtime.GOOS == "windows"

err := WaitTimeout(c, timeout)

if runtime.GOOS == "windows" {
var buf bytes.Buffer
Copy link
Contributor

@sparrc sparrc Aug 10, 2016

Choose a reason for hiding this comment

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

put this logic within the if-statement in it's own function, then unit test it and put a comment above the function explaining what it's doing.

@butitsnotme
Copy link
Contributor Author

@sparrc inputs.exec does not call internal.CombinedOutputTimeout(), it calls exec.Command() directly, only using internal for internal.RunTImeout().

Should it be using internal.CombinedOutputTimeout()? (Currently it does not include stderr in the output.)

@sparrc
Copy link
Contributor

sparrc commented Aug 11, 2016

yes, I think I was wrong, we should probably just strip the CRs from the exec plugin only, as you had it before.

@sparrc
Copy link
Contributor

sparrc commented Aug 17, 2016

looks good @butitsnotme, but you need to fix your CHANGELOG update. I would suggest just throwing away your changes to the changelog and rebasing, then adding your PR to the list again.

@steverweber
Copy link

+1 would like this in the RC

butitsnotme and others added 8 commits August 23, 2016 14:11
Added the option removecr to inputs.exec to remove all carraige returns
(CR, ASCII 0x0D, Unicode codepoint \u0D, ^M). The option is boolean and
not enabled if not present in the config file.
Moved the code to remove carriage returns from
plugins/inputs/exec/exec.go to internal/internal.go. Additionally
changed the conditional on which it gets applied from using a
configuration file option to checking if it is running on Windows.
Moved the carriage return removal back to the exec plugin. Added unit
testing for it. Fixed a bug (removing too many characters).
@butitsnotme
Copy link
Contributor Author

I've updated the changelog and rebased on master. I've tested the latest commit on Windows, it all works. The failed check appears to be unrelated (it looks like one of the docker images didn't initialize in time).

I would very much appreciate it if this could be incorporated into the 1.0 release, as without it the exec plugin is nearly useless on Windows.

@butitsnotme
Copy link
Contributor Author

The failed check looks like it has nothing to do with this... (Aerospike failed)

@sparrc
Copy link
Contributor

sparrc commented Aug 31, 2016

thx @butitsnotme

@sparrc sparrc closed this in 4886109 Aug 31, 2016
jackzampolin pushed a commit that referenced this pull request Sep 1, 2016
Added the option removecr to inputs.exec to remove all carraige returns
(CR, ASCII 0x0D, Unicode codepoint \u0D, ^M). The option is boolean and
not enabled if not present in the config file.

closes #1606

Updated CHANGELOG.md with information about removecr

Ran go fmt ./...

Moved removal of CRs to internal/internal.go

Moved the code to remove carriage returns from
plugins/inputs/exec/exec.go to internal/internal.go. Additionally
changed the conditional on which it gets applied from using a
configuration file option to checking if it is running on Windows.

Moved Carriage Return check to correct place

Moved the carriage return removal back to the exec plugin. Added unit
testing for it. Fixed a bug (removing too many characters).

Ran go fmt ./...

Reverted CHANGELOG to master

Updated Changelog
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.

3 participants