Skip to content

Issue #538 - Adding bash utility#690

Merged
caphrim007 merged 4 commits intoF5Networks:developmentfrom
jasonrahm:feature.util_bash
Sep 15, 2016
Merged

Issue #538 - Adding bash utility#690
caphrim007 merged 4 commits intoF5Networks:developmentfrom
jasonrahm:feature.util_bash

Conversation

@jasonrahm
Copy link
Copy Markdown

Updated file:
f5/bigip/tm/util/init.py

Added files:
f5/bigip/tm/util/Bash.py
f5/bigip/tm/util/test/test_bash.py
test/functional/tm/util/test_bash.py

@jasonrahm
Copy link
Copy Markdown
Author

@caphrim007 and @zancas this is ready for review.

Structurally I believe it is sound, but I wonder if we should allow any string in utilCmdArgs that doesn't start with "-c ..." since any other use of bash from REST would be useless. Thoughts appreciated.

@jasonrahm
Copy link
Copy Markdown
Author

@pjbreaux & @zancas, since you're in the neighborhood today, and thoughts on my previous comment?

@pjbreaux pjbreaux self-assigned this Sep 14, 2016
@pjbreaux
Copy link
Copy Markdown
Contributor

What would happen if I sent in FakeiControl.exec_cmd('run', utilsCmdArgs='env')?

@jasonrahm
Copy link
Copy Markdown
Author

likely bad things.

From: pjbreaux notifications@github.com
Reply-To: F5Networks/f5-common-python reply@reply.github.com
Date: Wednesday, September 14, 2016 at 5:03 PM
To: F5Networks/f5-common-python f5-common-python@noreply.github.com
Cc: Jason Rahm J.Rahm@F5.com, Author author@noreply.github.com
Subject: Re: [F5Networks/f5-common-python] Issue #538 - Adding bash utility (#690)

What would happen if I sent in FakeiControl.exec_cmd('run', utilsCmdArgs='env')?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/690#issuecomment-247170183, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AA0SS_eTwoQ6EpeauCZVLwM3dORvUhidks5qqG8WgaJpZM4J8OXS.

@jasonrahm
Copy link
Copy Markdown
Author

The actual answer is:

"/bin/env: /bin/env: cannot execute binary file "

So I'm guessing I should back off from testing for /bin/bash and just test for starting with /bin/ to catch any other unacceptable arguments passed to bash?

@jasonrahm
Copy link
Copy Markdown
Author

jasonrahm commented Sep 14, 2016

@jgruber requested the functionality and really only needs support for bash -c.....

But if there are other important use cases we need to allow, I want to be thorough.

@pjbreaux
Copy link
Copy Markdown
Contributor

I'm for adding the restriction if it will help protect users and notify them of the requirement. And when we get a user that needs to do something without -c, then we can handle it then.

@jasonrahm
Copy link
Copy Markdown
Author

Ok, I'll rework the code and resubmit tomorrow. Thanks for the feedback.

@jasonrahm
Copy link
Copy Markdown
Author

looks like I'm short a test case in coveralls @caphrim007, I'll investigate.

@jasonrahm
Copy link
Copy Markdown
Author

ok, @caphrim007, ready for review.

Copy link
Copy Markdown
Contributor

@caphrim007 caphrim007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment thread f5/bigip/tm/util/Bash.py

.. note::

This is an unnamed resource so it has not ~Partition~Name pattern
Copy link
Copy Markdown
Contributor

@caphrim007 caphrim007 Sep 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"no" instead of "not". We will fix this in a follow-up PR

'run',
utilCmdArgs='-c df -k')

# commandResult should b present with data from 'df -k'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"be". Again, we'll fix it in a follow-up PR

@caphrim007 caphrim007 merged commit cef1533 into F5Networks:development Sep 15, 2016
@jasonrahm
Copy link
Copy Markdown
Author

thanks, I'll fix those with next command in util.

@jasonrahm jasonrahm deleted the feature.util_bash branch October 6, 2016 20:53
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

Successfully merging this pull request may close these issues.

4 participants