-
Notifications
You must be signed in to change notification settings - Fork 166
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
hooks: add hook to run arbitrary commands #565
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome set of tests. One thing that might be nice is providing a timeout mechanism - this is a bit more of a PITA in python 2, only because you basically have to roll your own with signal. That said, I'm fine if that's not in this version.
stacker/hooks/command.py
Outdated
enabled. Default: false | ||
stdin (str, optional): | ||
String to send to the stdin of the command. Implicitly disables | ||
`intearctive`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
stacker/hooks/command.py
Outdated
shell: true | ||
""" | ||
|
||
if quiet: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we throw an error if quiet & capture are set? Not 100% sure it's required, but might be nice and avoid some confusion later.
stacker/hooks/command.py
Outdated
else: | ||
in_type = _devnull() | ||
|
||
if env is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if env:
should be good enough here
proc = Popen(command, stdin=in_type, stdout=out_err_type, | ||
stderr=out_err_type, env=env, **kwargs) | ||
try: | ||
out, err = proc.communicate(stdin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth a log statement here detailing the command being ran - probably debug?
BTW, awesome set of tests :) |
ping - @danielkza I think @ejholmes is likely to release stacker 1.3 this week. Just wanted to give you a heads up in case you want to get this in the next version. |
e7bf96b
to
7231794
Compare
@phobologic Rebased and updated for your suggestions. Sorry for taking so long, I actually completely forgot I had this PR open. |
@danielkza no worries, I figured :) Just wanted to give you the heads up! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
* hooks: add hook to run arbitrary commands * hooks: command: fail if `quiet` and `capture` options are both enabled * hooks: command: log command line before executing
No description provided.