search

alces-software / adminware

A sandbox CLI for running commands remotely across nodes
1 stars 0 forks source link

Combine the `run tool` and `run interactive` commands #131

Closed WilliamMcCumstie closed 5 years ago

WilliamMcCumstie commented 5 years ago

Now that the config specifies if a tool is interactive, it is possible to collapse run interactive into run tool. The config key has been changed from interactive_only to interactive see: https://github.com/alces-software/adminware/pull/131/commits/16373d6b18c6528555cfd9f18f24cd00da348162)

A bit of logic is required to do this, as interactive commands can only be ran on a single node. It also requires a separate running method as interactive commands should not be ran in threads and should skip reporting the results back.

Finally interactive commands are now logged to the db with exit code -3 and stubbed STDOUT and STDERR.

WilliamMcCumstie commented 5 years ago

This PR needs to be merged due to future changes that depend on it.

@DavidMarchant, Feel free to review this at a future date and open any issues you see in it.

DavidMarchant commented 5 years ago

My only issue is: what's this import doing at the bottom of Job? https://github.com/alces-software/adminware/pull/131/files#diff-bd6c0655c18b9a43b0f26346ae2e0ea6R112 AFAICT Batch isn't even used in the module

Other than that it's a retroactive LGTM on my end!

WilliamMcCumstie commented 5 years ago

So the reason why the import is at the bottom of the page is to prevent a circular import loop. Essentially:

  1. from models.job import Job preforms a number of actions: a. First it defines Job (even though it needs Batch at runtime) b. Then it tries to import Batch
  2. from models.batch import Batch then goes: a. Import Job: from models.job import Job. This preforms no additional tasks as Job has already been defined b. Defines Batch
  3. Control is passed back to the original import Job and now defines Batch within it
  4. This allows for Job to be returned from the original import

If the import is at the top of the page, step 2.a will try and repeat step 1 as Job hasn't been defined. This creates the circular import loop. Similar things can also happen in ruby.

DavidMarchant commented 5 years ago

Ah, cheers for explaining