Revamp the argument passing

[WilliamMcCumstie]

Arguments are now specified in the configs under an arguments key. The values for the arguments are specified on the command line forming key value pairs.

These key value pairs are now stored in the db under as ShellVariable models. Currently only alphanumeric values are supported as arguments.

These arguments are set as bash variables in the remote command. This will allow them to be used within the command. They are not exported into the environment and will not affect subshells.

This fixes #163

[WilliamMcCumstie]

Hmmm I didn't notice the capitalisation error on my first pass. Can you open an issue for it? Also make the issue clear that its capitalisation of the key not the value. This shouldn't hold up the release. I don't think we will need to support caps vs lower case, however some nice error handling could be added.

Any specified argument that contains any uppercase letters errors the system in a strange way. First of all is this intended, are uppercase letters disallowed? If not maybe there is some casting to lower/upper case going on at some point. If so the error messages should probably say so & the error conditionals should probably pick up on it rather than generating this stack trace I am getting

Yep you have a very good point there. I did notice this when I was implementing it, however I don't have a good solution for this. Best to wait for a use case and take it from there. ATM this is expected behaviour. As far as the error is concerned, it isn't an adminware issue and thus we shouldn't try and fix it.

It acts a little strange when executing namespaces. Tools that error when executed on their own without provided arguments don't when executed as part of an namespace, even if that namespace is comprised of only the offending tool. It's likely that this should be added as another error conditional within runner

This is the requirement specified in the issue and thus will be delivered as such. The limit character set is to prevent injection attacks. If more characters are required, then a new issue can be created for it.

Is exclusively alphanumeric characters enough leeway? If this is fine in your eyes that's good with me but what about your example with changing the refresh rate of top - periods are outlawed

[DavidMarchant]

Hmmm I didn't notice the capitalisation error on my first pass. Can you open an issue for it? Also make the issue clear that its capitalisation of the key not the value. This shouldn't hold up the release. I don't think we will need to support caps vs lower case, however some nice error handling could be added.

Yep! I'll chuck up an issue for this. For the mo would it be best to specify in the README that only lowercase keys are valid for the time being?

Yep you have a very good point there. I did notice this when I was implementing it, however I don't have a good solution for this. Best to wait for a use case and take it from there. ATM this is expected behaviour. As far as the error is concerned, it isn't an adminware issue and thus we shouldn't try and fix it.

Yeah, I understand that this is very difficult thing to solve properly as-is because of the way that runner is set up to just accept config(s). I'm fine with not handling this if you are however a sub-function in runner like:

def error_if_multiple_tools_without_arguments():
    matches = [c for c in configs if c.arguments()]
    if matches and len(configs) > 1:
        raise ClickException(......)

Would prevent all these issues bar the edgecase of running a namespace containing a only single tool that requires arguments. Up to you.

This is the requirement specified in the issue and thus will be delivered as such. The limit character set is to prevent injection attacks. If more characters are required, then a new issue can be created for it.

sure thing :+1:

[WilliamMcCumstie]

I wouldn't worry about making any additional changes at this point. There are always a million and one edge cases which aren't being handled. Better to wait for use cases then adding checking code or comments that will likely become out of date.

Barring any additional changes, is this PR approved to be merged?

[WilliamMcCumstie]

Possible open an issue about running multiple tools with args? I think their maybe a more intelligent way to handle this issue w/o raising an error

[DavidMarchant]

Yep! LGTM For reference the uppercase arguments issue has been created here: https://github.com/alces-software/adminware/issues/179 And yeah, I think a second issue is probably a good shout, I'll get on that