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

Add support for argument-checking in Actions #41

Open
nickthecook opened this issue Aug 28, 2020 · 1 comment
Open

Add support for argument-checking in Actions #41

nickthecook opened this issue Aug 28, 2020 · 1 comment

Comments

@nickthecook
Copy link
Owner

nickthecook commented Aug 28, 2020

When ops runs an action, it appends all command-line arguments to the command specified in the action config. Thus, it's easy to write actions that take arguments. For example:

actions:
  hello:
    command: echo Hello,
$ ops hello these are arguments.
Running 'echo Hello, these are arguments' from ops.yml in environment 'dev'...
Hello, these are arguments.

Also, if you're just running another executable or a script from the action, that executable or script can check its own arguments.

However, sometimes the action uses a specific subset of another executable's functionality. E.g., a script to run a command inside each of a set of containers shouldn't care what the command is or how many containers you tell it to run the command in, but if you have an ops action like console that needs to run a command in exactly one container, that action cares that exactly one argument is supplied.

It's also not obvious to a user that the command needs exactly one argument unless the developer writes it into the action description. And if the description contains the info, it could become stale and inaccurate if the command is updated and the description isn't (as unlikely as it is that a developer would do such a thing).

To help with this case, ops could take specifications like this:

actions:
  console:
    args:
      container_name:
        description: the name of one of the ops test containers
        mandatory: yes
    extra_args_allowed: no
    command: bin/run.sh bash "$container_name"

ops would assign its first command-line argument to the variable $container_name.

This approach:

  • documents the command for the user
  • enforces the documented usage, to prevent drift
  • might be flexible enough to allow most basic actions to get command argument checking

The default for extra_args_allowed should be true (thanks to YAML, a value of yes evaulates to true), and it should work on commands that do not define args.

Alternative, shorter format

An alternative format could also be supported, to make it less onerous to add argument-checking, and leave ops.yml a little smaller:

actions:
  console:
    args:
      - container_name
    extra_args_allowed: no
    command: bin/run.sh bash "$container_name"

Solution for argument interpolation

This approach may also partially solve #10. It would still be nice to have unnamed arguments interpolated into the command, but anyone wanting that could get something like it in most cases by using this functionality. Naming an argument is a bit more work to add to an action that just putting $1 in a command string, but it would work.

Implementation

There are a couple of ways to implement this, at a high level:

  1. update the string before executing the command

This method would have ops gsub all occurrences of $container_name in the command string before executing the command.

Pros: ops could warn if an argument was specified but not used in the command string
Cons: would evaluate all variable references regardless of quoting, which may lead to unexpected results

  1. set environment variables for each arg

This method would have ops set the environment variable "$container_name` to the value of its first argument before executing the command, and execute the command unmodified.

Pros: this would respect shell quoting; e.g. variable references inside single quotes would not be evaluated by the shell
Cons: ops could still warn about unused args, but it wouldn't be accurate if an arg reference occurred inside single-quotes


I feel that the second option is better via the Principle of Least Surprise, since someone writing a shell command wouldn't expect a variable reference inside single-quotes to be evaluated. The ability to warn about unused argument references is of questionable value, since perhaps the arg could be an environment variable that is not used in the command string, but is used by the script or command that it calls.

@nickthecook
Copy link
Owner Author

nickthecook commented Sep 7, 2020

Maybe required instead of mandatory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant