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

Fixed arguments being passed to utils.Command for Command Notifier #63

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

ralfonso-directnic
Copy link

The way the cmd string was being parsed result in utils.Command getting the following call
utils.Command("binary-to-call",[]string{"binary-to-call other args here"}, this properly removes the duplicate

The way the cmd string was being parsed result in utils.Command getting the following call 
utils.Command("binary-to-call",[]string{"binary-to-call other args here"}, this properly removes the duplicate
@ralfonso-directnic
Copy link
Author

After more investigation I moved the runCommand functionality to writing the command to a temp file and executing it. The existing method of parsing strings was inadequate when using quoted strings and was prone to failure

@jemand771
Copy link
Member

First of all: hi and thank you for you contribution :)

can you please give some more details on what impact the previous behaviour had to the outside? what do you have to set up (as a statping user) to get this to break?

As you might have seen in other conversations, 2 out of our 3 maintainers (including me) aren't familiar with go, so we kind of have to rely on your explaination and details here ^^

my first thought just from from what you've said so far: isn't writing to a tempfile just to execute it a bit hacky?

@jemand771 jemand771 added the Bug Something isn't working label Oct 13, 2021
@jonathanrbarney
Copy link

Addressed with #120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants