-
Notifications
You must be signed in to change notification settings - Fork 471
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
remote: fix connhelpers with custom dialer #2327
Conversation
With the new dial-stdio command the dialer is split from `Client` function in order to access it directly. This breaks the custom connhelpers functionality as support for connhelpers is a feature of the default dialer. If client defines a custom dialer then only it is used without extra modifications. This means that remote driver dialer needs to detect the connhelpers on its own. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
I think a custom test is fine, I will open a PR with this test to make it fail with current master. |
52c454d
to
73e04c9
Compare
2e85fbb
to
05b3229
Compare
05b3229
to
d9ba74a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A few comments on some refactoring ideas that I think would be minor and good to implement, but nothing blocking.
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
d9ba74a
to
b1490ed
Compare
fix #2324
closes #2329
With the new dial-stdio command the dialer is split from
Client
function in order to access it directly.This breaks the custom connhelpers functionality
as support for connhelpers is a feature of the default dialer. If client defines a custom dialer then only it is used without extra modifications. This means that remote driver dialer needs to detect the
connhelpers on its own.
The relevant default dialer addition is in https://github.com/moby/buildkit/blob/a38011b9f57db4b805e6bb0322d7d923b341bc6e/client/client.go#L109-L115 .
@cpuguy83 @jsternberg
@crazy-max For test, should we have one custom test just for connhelpers or maybe just add extra config in the matrix that runs remote driver with a
docker-container://
endpoint address?