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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

changed command execution logic. Closes #7 #14

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"os"
"os/exec"
"runtime"
"strings"

"github.com/yosssi/goat/consts"
"github.com/yosssi/goat/context"
Expand Down Expand Up @@ -63,13 +62,7 @@ func handleJobs(jobsC <-chan context.Job) {
func executeTasks(tasks []*context.Task, watcher *context.Watcher) {
for _, task := range tasks {
command := task.Command
tokens := strings.Split(command, " ")
name := tokens[0]
var cmdArg []string
if len(tokens) > 1 {
cmdArg = tokens[1:]
}
cmd := exec.Command(name, cmdArg...)
cmd := exec.Command("/bin/bash", "-c", command)
Copy link

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?

Copy link
Contributor Author

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.

Copy link

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.

Copy link
Contributor Author

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

  1. detecting the os/shell and switching how the command is run. (using runtime.GOOS maybe)
  2. have an attribute in the goat yml/json file to indicate the os/shell.

I really dont want to over complicate stuff though

Copy link

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?

Copy link
Contributor Author

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"

Copy link

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"
}

Copy link
Contributor Author

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.

Copy link

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...)

Copy link
Contributor Author

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.

cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
if task.Nowait {
Expand Down