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.

Sign up for GitHub

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

Jump to bottom

Adds Support for TCP Hijack in execStartStream #63

Open
wants to merge 7 commits into
base: 1.x
Choose a base branch
from
Open

Adds Support for TCP Hijack in execStartStream #63

wants to merge 7 commits into from

Conversation

bosunski
Copy link

Summary of changes

  • Adds $hijak parameter to exectStartsStream
  • Configures the browser based on $hijak

Example Usage

$client->execCreate('bash', 'bash', true, true, true, true)
            ->then(function($info) use ($client) {
                $client->execStartStream($info['Id'], true,'stderr', true)->then(function (UpgradedResponse $e) use ($deferred) {
                    $stream = $e->getConnection();
                    $stream->on('data', function ($chunk) {
                        echo $chunk;
                    });

                    $stream->write("ls\r"); // list directories
                });
            });

clue
clue reviewed Aug 12, 2020
View reviewed changes
Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@bosunski Thanks for looking into this, I think this is an excellent first step to discuss this feature in mroe detail!

I love the feature, but I'm not sure about the API in its current form.

The execStartStream() currently returns a ReadableStreamInterface (read-only).

It's my understanding the new API that supports writable streams should return a DuplexStreamInterface instead.

The DuplexStreamInterface extends the ReadableStreamInterface, so we could make this the return value of the same function or it might be better to come up with a dedicated API endpoint, I'm currently undecided.

What do you think about this? 👍

@bosunski
Copy link
Author

bosunski commented Aug 20, 2020 •
edited
Loading

@clue I'm thinking 🤔 since the Hijak feature is still part of the /exec/{exec}/start API and only surfaces when the Hijak is configured, we can just have the execStartStream return a DuplexStreamInterface which, as you've said, will cover for the ReadableStreamInterface also.

I initially added execStartStreamUpgrade as a public interface, but I noticed the same parameters are being passed for it as well as execStartStream which is more like some repetition there. Which lead me to merge the 2 methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clue clue clue left review comments

Assignees
No one assigned
Labels
help wanted new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support writable streams (process STDIN)
2 participants
@bosunski @clue