Allow Custom CLI commands to be any (complex) shell commands

[stoned]

what

• Allow Custom CLI commands to be any (complex) shell commands

why

• Sometimes it could be handy to inline small, or no so small, shell scripts in custom CLI command definition
• This may help with custom cli commands: Cannot access outputted plan file or atmos workspace name #215

references

• https://github.com/mvdan/sh
  • FYI it is used in a similar context by Task

Misc

• With the proposed changes, the ExecuteShellCommandAndReturnOutput is no longer used and may be removed.
• I am also pondering to modify executeWorkflowSteps in a similar way. Would that make sense ?

[aknysh]

@stoned thanks again.

Can you please explain what issues you are solving and what is not possible with the current implementation?

Note that currently command steps support complex scripts using https://yaml-multiline.info, e.g.

    steps:
    - touch ${KUBECONFIG}
    - >
      aws eksupdate-kubecfg 
      --cluster-name {{ .Flags.environment }}-{{ .Flags.stage }}-eks-cluster 
      --aws-profile xxx-gbl-{{ .Flags.stage }}-helm

If the library adds something what is not currently supported, we'll consider it.
We don't want to add a 3rd-party library just b/c it's fancy. Many issues with those 3rd-party libraries, they have bugs we we would encounter later in some cases, they change, they break, they go obsolete (we fixed a few issues like that recently). The more we have them, the more difficult to maintain the whole system.

[stoned]

The function ExecuteShellCommand() does not fork a shell to execute its provided command. Also executeCustomCommand() execute a simple word splitting with strings.Fields() on the steps values. Both these behaviors prevent using a complex shell command, which has nothing to do with a long or somewhat "hard" to read command.
The example command you provided above is still a simple shell command, as opposed to a complex one like for example date ; echo foo. If you try to specify the latter in a step, you will end up with an error like the following:

Executing command:
/usr/bin/date ; echo foo
date: extra operand ‘echo’
Try 'date --help' for more information.
exit status 1

The support of any shell command without using a library like mvdan/sh if of course possible, and I can provide an alternate implementation if you prefer.

[aknysh]

The function ExecuteShellCommand() does not fork a shell to execute its provided command. Also executeCustomCommand() execute a simple word splitting with strings.Fields() on the steps values. Both these behaviors prevent using a complex shell command, which has nothing to do with a long or somewhat "hard" to read command.
The example command you provided above is still a simple shell command, as opposed to a complex one like for example date ; echo foo. If you try to specify the latter in a step, you will end up with an error like the following:

That's correct, commands separated by ; (date ; echo foo) are not supported (it's Go limitations) (for that we have steps. but it's a separate topic).

I'll review the PR little bit later.

Did you test it by running on the command-line? For example, all the currently defined commands in https://github.com/cloudposse/atmos/blob/master/atmos.yaml#L69 should work.

Thanks

[stoned]

The commands mentioned do work, albeit some defined stacks or component do have problems, like for example:

$ atmos tf plan -s tenant1-ue2-dev vpc

Found duplicate config for the component 'vpc' for the stack 'tenant1-ue2-dev' in the files: orgs/cp/tenant1/dev/us-east-2-extras, orgs/cp/tenant1/dev/us-east-2.
Check that all context variables in the stack name pattern '{tenant}-{environment}-{stage}' are correctly defined in the files and not duplicated.
Check that all imports are valid.

exit status 1

[stoned]

The commands mentioned do work, albeit some defined stacks or component do have problems, like for example:

And the example could now use a bit of quoting as a shell process is now involved in the steps execution...

[stoned]

The support of any shell command without using a library like mvdan/sh if of course possible, and I can provide an alternate implementation if you prefer.

A simpler and incomplete (there is no Windows support for example, unless "Git Bash"/MKSYS/WSL/whatever is used) alternative implementation could be along these lines. Would you prefer something like that ?

[aknysh]

The commands mentioned do work, albeit some defined stacks or component do have problems, like for example:

$ atmos tf plan -s tenant1-ue2-dev vpc

Found duplicate config for the component 'vpc' for the stack 'tenant1-ue2-dev' in the files: orgs/cp/tenant1/dev/us-east-2-extras, orgs/cp/tenant1/dev/us-east-2.
Check that all context variables in the stack name pattern '{tenant}-{environment}-{stage}' are correctly defined in the files and not duplicated.
Check that all imports are valid.

exit status 1

the component vpc shows that behavior on purpose, it was done to detect this exact error.

Please use infra/vpc component for testing

[aknysh]

@stoned you removed

commandToRun := os.ExpandEnv(commandTmpl)

which is used to replace ${var} or $var in the command steps at runtime according to the values of the current environment variables.

Does interp.Env(expand do the same thing?

Also, we prob need to simplify

interp.Env(expand.ListEnviron(append(os.Environ(), env...)...)),

by using a local var b/c it's difficult to read

[stoned]

Exactly, mvdan.cc/sh/v3/interp.Env() will setup the environment to use: there is no need to have os.ExpandEnv() making its own substitutions.
And, yes, I will rewrite the call to interp.Env() to be, hopefully, simpler to read.

[aknysh]

@stoned thanks again for the PR, LGTM, a few nitpicks, please see comments

[stoned]

the component vpc shows that behavior on purpose, it was done to detect this exact error.

Please use infra/vpc component for testing

OK, got it.

[stoned]

@stoned thanks again for the PR, LGTM, a few nitpicks, please see comments

@aknysh I pushed a new commit in the branch/PR with some refactoring.

[aknysh]

thanks @stoned