Skip to content

Add ability to stream command output as a response (fixes conflict #443)#549

Open
lostsnow wants to merge 1 commit into
adnanh:masterfrom
lostsnow:feature/stream-output-as-response
Open

Add ability to stream command output as a response (fixes conflict #443)#549
lostsnow wants to merge 1 commit into
adnanh:masterfrom
lostsnow:feature/stream-output-as-response

Conversation

@lostsnow

@lostsnow lostsnow commented Aug 4, 2021

Copy link
Copy Markdown

Just fix conflict pr #443, thanks @milolav

@manukall

Copy link
Copy Markdown

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

wille commented Feb 15, 2022

Copy link
Copy Markdown

Much needed. Please merge.

@sto

sto commented Sep 12, 2022

Copy link
Copy Markdown

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

Ftrybe commented Apr 3, 2024

Copy link
Copy Markdown

Any news about this PR?

@adnanh

adnanh commented Apr 13, 2024

Copy link
Copy Markdown
Owner

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

adnanh commented Apr 16, 2024

Copy link
Copy Markdown
Owner

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

Ftrybe commented Jun 8, 2024

Copy link
Copy Markdown

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

@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