-
Notifications
You must be signed in to change notification settings - Fork 573
driver: check history capability #1846
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
Conversation
driver.CacheExport: true, | ||
driver.MultiPlatform: true, // Untested (needs multiple Driver instances) | ||
driver.CacheExport: true, | ||
driver.MultiPlatform: true, // Untested (needs multiple Driver instances) |
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.
Unrelated, but I think this comment is out-of-date now?
I think this is tested now, at least we have docs for this scenario?
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.
0831fef
to
7805f9b
Compare
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
7805f9b
to
8c65e4f
Compare
} | ||
cl, err := c.ControlClient().ListenBuildHistory(ctx, &controlapi.BuildHistoryRequest{ | ||
ActiveOnly: true, | ||
Ref: "buildx-dummy-ref", // dummy ref to check if the server supports the API |
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.
"buildx-test-history-api-feature" so that if you look at it from server side you understand where the request is coming from.
res = true | ||
checkErrF := func(err error) { | ||
if s, ok := grpcerrors.AsGRPCStatus(err); ok { | ||
if s.Code() == codes.Unimplemented { |
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.
What error except for EOF in here means there is support?
checkErrF(err) | ||
return | ||
} | ||
_, err = cl.Recv() |
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.
Client should be read until it is empty. It can be closed/canceled if there is an issue.
func (d *Driver) Features() map[driver.Feature]bool { | ||
func (d *Driver) Features(ctx context.Context) map[driver.Feature]bool { | ||
var historyAPI bool | ||
c, err := d.Client(ctx) |
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.
This call is expensive. It makes an new docker exec
.
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.
The client is cached
Lines 157 to 162 in a906149
func (d *cachedDriver) Client(ctx context.Context) (*client.Client, error) { | |
d.once.Do(func() { | |
d.client, d.err = d.Driver.Client(ctx) | |
}) | |
return d.client, d.err | |
} |
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.
That is not the struct that's get called in the line 392.
related to #1841
Adds history API feature check for each driver. Also adds cache for driver features.