Revamp the argument passing #175

WilliamMcCumstie · 2018-11-19T14:26:32Z

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

Shell variables are essentially key value pairs that are associated with a `Batch`. It isn't a great idea to store lists within a db and as such each variable will be stored in its own table. The basic design of ShellVariable has been modelled of Job (w/o the running code)

To allow for greater flexibility in the arguments passing, the individual config files now dictate their arguments list. Note: The `config.args()` method is the safe version which handles the case when no arguments are specified. The `config.arguments` attribute returns the raw version from the config BUG FIX: It should have been `arg_n` not `arg_name`

There are going to be a couple of steps involved when creating a `batch`. Thus it is easier if they are abstracted away into a single function

It is only going to be used within the run method and as such should be located with it

The ShellVariable models need to be added to the batches in a similar way to Jobs. This does take some effort to combined the shell variables as key value pairs

This will prevent threading issue with SQLAlchemy

The shell variables are set as bash variables before the command is ran. This allows then to be substituted into the config command.

This is to prevent a injection attack through the shell variables

This allows the error to be caught by clicks top level error handler

DavidMarchant

The code seems great though I have a couple of comments/questions about the functionality:

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

adminware> run cmd1 -n node dsadas  sadsada                                                       
Traceback (most recent call last):                                                                
  File "src/adminware", line 7, in <module>                                                       
    adminware()                                                                                   
  File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 722, in __call__     
    return self.main(*args, **kwargs)                                                             
  File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 697, in main         
    rv = self.invoke(ctx)                                                                         
  File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 1066, in invoke      
    return _process_result(sub_ctx.command.invoke(sub_ctx))                                       
  File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 895, in invoke       
    return ctx.invoke(self.callback, **ctx.params)                                                
  File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 535, in invoke       
    return callback(*args, **kwargs)                                                              
  File "/tmp/adminware/src/appliance_cli/sandbox.py", line 31, in sandbox                         
    allow_internal_commands=False,                                                                
  File "/tmp/adminware/venv/src/click-repl/click_repl/__init__.py", line 194, in repl             
    group.invoke(ctx)                                                                             
  File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 1066, in invoke      
    return _process_result(sub_ctx.command.invoke(sub_ctx))                                       
  File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 1066, in invoke      
    return _process_result(sub_ctx.command.invoke(sub_ctx))                                       
  File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 895, in invoke       
    return ctx.invoke(self.callback, **ctx.params)                                                
  File "/tmp/adminware/venv/lib/python3.5/site-packages/click/core.py", line 535, in invoke       
    return callback(*args, **kwargs)                                                              
  File "/tmp/adminware/venv/lib/python3.5/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)                                              
  File "/tmp/adminware/src/appliance_cli/command_generation.py", line 136, in click_callback      
    params[arg_name] for arg_name in arguments.keys()                                             
  File "/tmp/adminware/src/appliance_cli/command_generation.py", line 136, in <listcomp>          
    params[arg_name] for arg_name in arguments.keys()                                             
KeyError: 'INPUT'

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

Barring the above and a suggestion about a comment, this looks good

DavidMarchant · 2018-11-19T15:20:31Z

README.md

@@ -123,6 +123,14 @@ interactive: True
 # Runs the command in reporting mode. The standard output of the remote command will
 # be dumped to the screen instead of the exit code.
 report: True
+
+# The following list form the arguments to the command line. They are set as bash shell


I think this line could do with some clearing up - either 'The following list forms the arguments sent to the command line' or 'The following list forms the arguments for the command line'

WilliamMcCumstie · 2018-11-19T16:32:31Z

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 · 2018-11-19T17:14:14Z

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 👍

WilliamMcCumstie · 2018-11-19T17:31:54Z

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 · 2018-11-19T17:33:17Z

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 · 2018-11-19T17:34:14Z

Yep! LGTM
For reference the uppercase arguments issue has been created here: #179
And yeah, I think a second issue is probably a good shout, I'll get on that

WilliamMcCumstie added 10 commits November 19, 2018 14:06

Extract the batch building to an inner function

1b52fab

There are going to be a couple of steps involved when creating a `batch`. Thus it is easier if they are abstracted away into a single function

Make the get_confirmation an inner function

e169aec

It is only going to be used within the run method and as such should be located with it

Build the shell variables on the batches

c0d23cd

The ShellVariable models need to be added to the batches in a similar way to Jobs. This does take some effort to combined the shell variables as key value pairs

Ensure the models are loaded in the main thread

dcc05d2

This will prevent threading issue with SQLAlchemy

Set the shell variables in the command

624ad62

The shell variables are set as bash variables before the command is ran. This allows then to be substituted into the config command.

Ensure the argument values are alphanumeric

214494f

This is to prevent a injection attack through the shell variables

Catch the validation error as a click error

13439c3

This allows the error to be caught by clicks top level error handler

Update the README

65912a9

WilliamMcCumstie requested a review from DavidMarchant November 19, 2018 14:26

WilliamMcCumstie mentioned this pull request Nov 19, 2018

Fix the status command #176

Merged

DavidMarchant reviewed Nov 19, 2018

View reviewed changes

Tweak the readme comment

e7a7726

DavidMarchant approved these changes Nov 19, 2018

View reviewed changes

DavidMarchant merged commit f2f7e93 into develop Nov 19, 2018

DavidMarchant deleted the feature/argument-vars branch November 19, 2018 17:36

DavidMarchant mentioned this pull request Nov 19, 2018

Inteligently reconcile running namespaces with tool arguments #180

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp the argument passing #175

Revamp the argument passing #175

WilliamMcCumstie commented Nov 19, 2018

DavidMarchant left a comment

DavidMarchant Nov 19, 2018

WilliamMcCumstie Nov 19, 2018

WilliamMcCumstie commented Nov 19, 2018 •

edited

Loading

DavidMarchant commented Nov 19, 2018

WilliamMcCumstie commented Nov 19, 2018

WilliamMcCumstie commented Nov 19, 2018

DavidMarchant commented Nov 19, 2018

Revamp the argument passing #175

Revamp the argument passing #175

Conversation

WilliamMcCumstie commented Nov 19, 2018

DavidMarchant left a comment

Choose a reason for hiding this comment

DavidMarchant Nov 19, 2018

Choose a reason for hiding this comment

WilliamMcCumstie Nov 19, 2018

Choose a reason for hiding this comment

WilliamMcCumstie commented Nov 19, 2018 • edited Loading

DavidMarchant commented Nov 19, 2018

WilliamMcCumstie commented Nov 19, 2018

WilliamMcCumstie commented Nov 19, 2018

DavidMarchant commented Nov 19, 2018

WilliamMcCumstie commented Nov 19, 2018 •

edited

Loading