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 shell-mode to shell plugin #765

Merged
merged 1 commit into from Dec 5, 2018

Conversation

Projects
None yet
4 participants
@ribab
Contributor

ribab commented Nov 20, 2018

Now you can run shell plugin just like you are running shell!

[TestRun:Example]
plugin = Shell
shell_mode = true
command = #!/bin/bash
          echo "hi"
          SOMEVAR=2
          echo somevar: $$SOMEVAR
          echo somevar+2: $$($$SOMEVAR + 2)

@ribab ribab requested review from beardeddog, DebRez and noahv Nov 20, 2018

@ribab ribab self-assigned this Nov 20, 2018

@ribab ribab added the Python Client label Nov 20, 2018

@beardeddog

can we have a BAT test added for this too?

@DebRez

This comment has been minimized.

Member

DebRez commented Nov 26, 2018

Looks good to me.

@@ -51,6 +51,7 @@ def __init__(self):
self.options['allocate_cmd'] = (None, "Command to use for allocating nodes from the resource manager")
self.options['deallocate_cmd'] = (None, "Command to use for deallocating nodes from the resource manager")
self.options['asis_target'] = (None, "Specifies name of asis_target being built. This is used with \"ASIS\" keyword to determine whether to do anything.")
self.options['shell_mode'] = ("false", "Use shlex or shell-mode for parsing?")

This comment has been minimized.

@PeterGottesman

PeterGottesman Nov 28, 2018

Contributor

Why not default to the boolean False instead of "false"? That way parseOptions will correctly translate the value to a boolean for you.

This comment has been minimized.

@ribab

ribab Nov 29, 2018

Contributor

Hmm I didn't know about this parseOptions feature. I was using "lower()" to parse the options myself.

Are you saying that with boolean False, if I type "false" in the INI file it would still parse to be false as a boolean? I was thinking that it would give me a string "False" which would evaluate to True...

This comment has been minimized.

@PeterGottesman

PeterGottesman Nov 30, 2018

Contributor

Yep, when you call parseOptions it calls __convert_value which converts based on the type of the default value.

def __convert_value(self, opt, inval):
if opt is None or type(opt) is str:
return 0, inval
elif type(opt) is bool:
if type(inval) is bool:
return 0, inval
elif type(inval) is str:
if inval.lower() in ['true', '1', 't', 'y', 'yes']:
return 0, True
else:
return 0, False

@ribab ribab force-pushed the ribab:topic/shell_mode branch from 2451cb1 to 3cb6ca1 Dec 5, 2018

Add shell-mode to shell plugin
Now you can run shell plugin just you are running shell!

[TestRun:Example]
plugin = Shell
shell_mode = true
command = #!/bin/bash
          echo "hi"
          SOMEVAR=2
          echo somevar: $$SOMEVAR
          echo somevar+2: $$($$SOMEVAR + 2)

Signed-off-by: Richard Barella <richard.t.barella@intel.com>

@ribab ribab force-pushed the ribab:topic/shell_mode branch from 3cb6ca1 to 4709bbc Dec 5, 2018

@ribab

This comment has been minimized.

Contributor

ribab commented Dec 5, 2018

I added a BAT test, and I followed Peter's suggestion to use the __convert_option feature.

Ready for another review

@beardeddog

ok, looks good, thanks

@ribab ribab merged commit 69882fd into open-mpi:master Dec 5, 2018

3 checks passed

Commit email checker Good email address. Yay!
Signed-off-by checker Commit is signed off. Yay!
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment