Skip to content
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

Merged
merged 3 commits into from
Jun 2, 2018

Conversation

danielkza
Copy link
Contributor

No description provided.

Copy link
Member

@phobologic phobologic left a 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.

enabled. Default: false
stdin (str, optional):
String to send to the stdin of the command. Implicitly disables
`intearctive`.
Copy link
Member

Choose a reason for hiding this comment

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

typo

shell: true
"""

if quiet:
Copy link
Member

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.

else:
in_type = _devnull()

if env is not None:
Copy link
Member

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)
Copy link
Member

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?

@phobologic
Copy link
Member

BTW, awesome set of tests :)

@phobologic
Copy link
Member

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.

@danielkza
Copy link
Contributor Author

danielkza commented Apr 8, 2018

@phobologic Rebased and updated for your suggestions. Sorry for taking so long, I actually completely forgot I had this PR open.

@phobologic
Copy link
Member

@danielkza no worries, I figured :) Just wanted to give you the heads up!

Copy link
Member

@phobologic phobologic left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@phobologic phobologic merged commit d4eccc3 into cloudtools:master Jun 2, 2018
phrohdoh pushed a commit to phrohdoh/stacker that referenced this pull request Dec 18, 2018
* 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
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.

2 participants