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

Add streaming command support. #251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wrouesnel
Copy link
Contributor

@wrouesnel wrouesnel commented May 17, 2018

Implements #250.
Add options

  • stream-stdout-in-response
  • stream-stdout-in-response-on-error
  • stream-command-kill-grace-period-seconds

to allow defining webhooks which dynamically stream large content back to the
requestor. This allows the creation of download endpoints from scripts, i.e.
running a git archive command or a database dump from a docker container,
without needing to buffer up the original.

Notes

This includes a refactor and clean up hookHandler to be somewhat more linear then the original was due to the added complexity of handling the deferred behavior of command streaming.

@wrouesnel wrouesnel force-pushed the streaming_responses branch 3 times, most recently from 807e96a to 1991a12 Compare May 23, 2018 01:57
@wrouesnel wrouesnel changed the title [WIP] Add streaming command support. Add streaming command support. May 25, 2018
@adnanh
Copy link
Owner

adnanh commented Sep 14, 2018

Nice, I'll have to take some time to review the refactor changes you made to the hookHandler.

@gdubicki
Copy link
Contributor

gdubicki commented Oct 9, 2018

We have approval, so can we have it merged, @adnanh @wrouesnel ?

Add options
- `stream-stdout-in-response`
- `stream-stdout-in-response-on-error`
- `stream-command-kill-grace-period-seconds`

to allow defining webhooks which dynamically stream large content back to the
requestor. This allows the creation of download endpoints from scripts, i.e.
running a `git archive` command or a database dump from a docker container,
without needing to buffer up the original.
@wrouesnel
Copy link
Contributor Author

A long overdue rebase and merge!

@VojtechMyslivec
Copy link

I would really appreciate this feature in CI/CD. Is it possible to review and merge this, since it seems to be implemented already 🙏

Thanks a lot for your work 👍

@Jackenmen
Copy link

I don't know if Adnan will be looking at this any time soon but just as an FYI for the PR author, this is now conflicting again.

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.

None yet

6 participants