8000 Allow Custom CLI commands to be any (complex) shell commands by stoned · Pull Request #260 · cloudposse/atmos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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

Jump to bottom
Merged
merged 8 commits into from
Nov 30, 2022
Merged

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

merged 8 commits into from
Nov 30, 2022

Conversation

stoned
Copy link
Contributor
@stoned stoned commented Nov 20, 2022

what

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

why

references

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 ?

@stoned
Allow Custom CLI commands to be any (complex) shell commands
f53b981 
... using mvdan.cc/sh/v3's shell parser and interpreter.

Also execute "valueCommand commands" the same way when defining
environment variables for custom CLI commands.
@stoned stoned requested review from a team as code owners November 20, 2022 14:44
@stoned stoned requested review from Gowiem and korenyoni November 20, 2022 14:44
@stoned @nitrocode
Use full package reference
f3f948d 
Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
aknysh
aknysh reviewed Nov 20, 2022
View reviewed changes
Copy link
Member
@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author
stoned commented Nov 20, 2022

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
Copy link
Member
aknysh commented Nov 20, 2022

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
Copy link
Contributor Author
stoned commented Nov 20, 2022

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
Copy link
Contributor Author
stoned commented Nov 20, 2022

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
Copy link
Contributor Author
stoned commented Nov 20, 2022

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 ?

@osterman osterman added the enhancement New feature or request label Nov 23, 2022
@aknysh
Copy link
Member
aknysh commented Nov 28, 2022

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
aknysh reviewed Nov 28, 2022
View reviewed changes
internal/exec/shell_utils.go Outdated

runner, err := interp.New(
interp.Dir(dir),
interp.Env(expand.ListEnviron(append(os.Environ(), env...)...)),
Copy link
Member
@aknysh aknysh Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
aknysh reviewed Nov 28, 2022
View reviewed changes
Copy link
Member
@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

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

@stoned
Copy link
Contributor Author
stoned commented Nov 28, 2022

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
Copy link
Contributor Author
stoned commented Nov 30, 2022

@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
aknysh approved these changes Nov 30, 2022
View reviewed changes
Copy link
Member
@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

thanks @stoned

@aknysh aknysh merged commit 4bbe53b into cloudposse:master Nov 30, 2022
@stoned stoned mentioned this pull request Dec 15, 2022
Merged
@stoned stoned deleted the sh-as-custom-cmds branch January 1, 2023 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nitrocode nitrocode nitrocode left review comments

@aknysh aknysh aknysh approved these changes

@Gowiem Gowiem Awaiting requested review from Gowiem Gowiem was automatically assigned from cloudposse/contributors

@korenyoni korenyoni Awaiting requested review from korenyoni korenyoni was automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stoned @aknysh @nitrocode @osterman
0