Skip to content

Conversation

crazy-max
Copy link
Member

related to #1841

Adds history API feature check for each driver. Also adds cache for driver features.

driver.CacheExport: true,
driver.MultiPlatform: true, // Untested (needs multiple Driver instances)
driver.CacheExport: true,
driver.MultiPlatform: true, // Untested (needs multiple Driver instances)
Copy link
Collaborator

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

jedevc
jedevc previously approved these changes May 26, 2023
@jedevc jedevc self-requested a review May 26, 2023 09:00
@crazy-max crazy-max dismissed jedevc’s stale review May 26, 2023 09:04

As discussed needs rework

crazy-max added 3 commits May 30, 2023 20:13
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>
@crazy-max crazy-max force-pushed the history-capability branch from 7805f9b to 8c65e4f Compare May 30, 2023 18:13
@crazy-max crazy-max requested a review from jedevc May 30, 2023 18:16
@crazy-max crazy-max merged commit e7034f6 into docker:master May 30, 2023
@crazy-max crazy-max deleted the history-capability branch May 30, 2023 18:25
}
cl, err := c.ControlClient().ListenBuildHistory(ctx, &controlapi.BuildHistoryRequest{
ActiveOnly: true,
Ref: "buildx-dummy-ref", // dummy ref to check if the server supports the API
Copy link
Member

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 {
Copy link
Member

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()
Copy link
Member

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)
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

The client is cached

buildx/driver/manager.go

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
}
so we are probably fine here no?

Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

3 participants