Skip to content

hooks: add hook to run arbitrary commands#565

Merged
phobologic merged 3 commits into
cloudtools:masterfrom
Cobliteam:add-command-hook
Jun 2, 2018
Merged

hooks: add hook to run arbitrary commands#565
phobologic merged 3 commits into
cloudtools:masterfrom
Cobliteam:add-command-hook

Conversation

@danielkza

Copy link
Copy Markdown
Contributor

No description provided.

@phobologic phobologic left a comment

Copy link
Copy Markdown
Member

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.

Comment thread stacker/hooks/command.py Outdated
enabled. Default: false
stdin (str, optional):
String to send to the stdin of the command. Implicitly disables
`intearctive`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo

Comment thread stacker/hooks/command.py
shell: true
"""

if quiet:

Copy link
Copy Markdown
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.

Comment thread stacker/hooks/command.py Outdated
else:
in_type = _devnull()

if env is not None:

Copy link
Copy Markdown
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

Comment thread stacker/hooks/command.py
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
Copy Markdown
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
Copy Markdown
Member

BTW, awesome set of tests :)

@phobologic

Copy link
Copy Markdown
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

danielkza commented Apr 8, 2018

Copy link
Copy Markdown
Contributor Author

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

@phobologic

Copy link
Copy Markdown
Member

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

@phobologic phobologic left a comment

Copy link
Copy Markdown
Member

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