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 ability to stream command output as a response (fixes conflict #443) #549

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

Conversation

lostsnow
Copy link

@lostsnow lostsnow commented Aug 4, 2021

Just fix conflict pr #443, thanks @milolav

@manukall
Copy link

would it be possible to get this merged? we've got a long running command, and only see the output after everything finished. streaming the output would be really great.

@wille
Copy link

wille commented Feb 15, 2022

Much needed. Please merge.

@sto
Copy link

sto commented Sep 12, 2022

Any news about this PR? The feature would be quite useful.

fjbarrena pushed a commit to kyso-io/webhook-scs that referenced this pull request Sep 21, 2023
For the s3import script seems useful to print the output as it happens,
as version 2.8.0 of webhook does not have this functionality we are
applying the PR 549 (adnanh/webhook#549) and
building the binary for alpine.
@Ftrybe
Copy link

Ftrybe commented Apr 3, 2024

Any news about this PR?

@adnanh
Copy link
Owner

adnanh commented Apr 13, 2024

I'll try to test this out next week, and hopefully get it merged if I don't run into any issues :)

@adnanh adnanh self-assigned this Apr 13, 2024
@adnanh adnanh self-requested a review April 13, 2024 21:33
@adnanh
Copy link
Owner

adnanh commented Apr 16, 2024

Hmm, in order to get the streaming to work, I had to set these headers in the response as well:

 w.Header().Set("Content-Type", "text/event-stream")
 w.Header().Set("Cache-Control", "no-cache")
 w.Header().Set("Connection", "keep-alive")

Without those headers I observed command output not being streamed, but rather buffered & spit out after the connection is closed.

Do you think this header override is acceptable?

Another question, should we also stream the output to the logs in parallel, maybe even introduce a separate setting "include-command-output-in-the-logs" (or something less verbose)? :)

@Ftrybe
Copy link

Ftrybe commented Jun 8, 2024

I think it's reasonable to override the headers if streaming requires it. Additionally, an include-command-output-in-the-logs option would be useful, since not everyone might want to enable this feature

@Srijan-SS02
Copy link

@adnanh any updates on this PR? Need this feature urgently, please let know if you are merging it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants