-
Notifications
You must be signed in to change notification settings - Fork 14
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
changed command execution logic. Closes #7 #14
base: master
Are you sure you want to change the base?
Conversation
cmdArg = tokens[1:] | ||
} | ||
cmd := exec.Command(name, cmdArg...) | ||
cmd := exec.Command("/bin/bash", "-c", command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't crossplatform solution, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No its not.... which is probably why its not merged yet. I am mostly on linux/mac and this solution works. I need environment variables to be parsed. I am open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about use of regexps? Not the final version of code, just the idea:
var envVar = regexp.MustCompile(`\${([A-Za-z0-9._\-]+)}`)
func processCommand(cmd string) string {
return envVar.ReplaceAllStringFunc(cmd, func(k string) string {
return os.Getenv(envVar.ReplaceAllString(k, "$1"))
})
}
And then update the existing code:
- command := task.Command
+ command := processCommand(task.Command)
tokens := strings.Split(command, " ")
name := tokens[0]
var cmdArg []string
if len(tokens) > 1 {
cmdArg = tokens[1:]
}
cmd := exec.Command(name, cmdArg...)
After that all values in a ${SOME_NAME}
form will be replaced by the appropriate environment variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its not just env variables, it could be another command as well, which could have a env variable inside.... i am pretty weary of using regex for that.... too many cases to check and it could get really complicated real fast.
I am personally running sub commands inside $() which caused the issue in the first place
I was thinking of maybe
- detecting the os/shell and switching how the command is run. (using
runtime.GOOS
maybe) - have an attribute in the goat yml/json file to indicate the os/shell.
I really dont want to over complicate stuff though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its not just env variables, it could be another command as well, which could have a env variable inside....
I'm not sure I'm following. Can you illustrate your words by an example of such command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like:
docker exec -it $(docker-compose ps -q $TEST) echo "hi"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what you want is support of shell commands, not just Environment variables (you didn't include this detail in the #14 PR's description, it is however in the title of #7 which I missed).
The right (cross-platform) way for adding support of env vars is os.ExpandEnv
(yeah, you don't even need to use regexps).
For subcommands I would go with the parsing of (
and )
. Not an easy fix, you'll have to use a stack data structure and parse & execute every subcommand one-by-one. But cross-platformability worth it.
For support of the shell commands, I would add a new key to the configuration file (what you've mentioned in your comment above). But definitely not break the work of the tool for users of other platforms.
Alternatively (if you do not have to use subcommands / shell commands too often), you can just create a shell script (e.g. smth.sh
):
docker exec -it $(docker-compose ps -q $TEST) echo "hi"
And then use it as follows:
{
"command": "sh ./smth.sh"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My master branch has this merged and kind of serves my purpose. I just thought a PR for the same might be good. I understand the cross compatibility issue but just threw it out here for what its worth.
If anybody else needs this feature too, I think adding an optional tag, like prefix_command
(at global scope would be good enough for me) to the yaml/json file would be the simplest and elegant solution without breaking cross platform support/functionality.
I am a "heavy" user of goat and really glad i found it. I have around 15 watchers and quite a few init tasks all of which needs subcommands/ env variables, writing script files for all of them would not be practical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've discovered a package for exactly this use-case: https://github.com/mattn/go-shellwords
It supports env vars, subcommands (that are run in SHELL but in a cross-platform way), quotes. The change required for its use is:
- tokens := strings.Split(command, " ")
+ tokens, err := shellwords.Parse(command)
+ if err != nil {
+ ...
+ }
name := tokens[0]
var cmdArg []string
if len(tokens) > 1 {
cmdArg = tokens[1:]
}
cmd := exec.Command(name, cmdArg...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome find.... let me check it out.
Is there anything happening with this PR? |
@TommyM Sorry I haven't had the time to do this. If you just need a version with this PR merged, plz take a look at my fork and its releases |
changed command execution logic to support commands with env variables