Set BuildKit's ExportedProduct variable to show useful errors in the future by tiborvass · Pull Request #37439 · moby/moby

tiborvass · 2018-07-11T22:17:20Z

This introduces a PRODUCT environment variable that is used to set a constant
at dockerversion.ProductName.

That is then used to set BuildKit's ExportedProduct variable in order to show
useful error messages to users when a certain version of the product doesn't
support a BuildKit feature.

Vendors buildkit to c3846bd

thaJeztah · 2018-07-11T22:21:07Z

full diff: moby/buildkit@9acf51e...c3846bd

thaJeztah · 2018-07-11T22:27:20Z

vendor/github.com/moby/buildkit/vendor.conf

double-checked, and these all match what's already in moby/vendor.conf

codecov · 2018-07-12T02:00:17Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a5495f2). Click here to learn what that means.
The diff coverage is 33.33%.

@@            Coverage Diff            @@
##             master   #37439   +/-   ##
=========================================
  Coverage          ?   34.95%           
=========================================
  Files             ?      610           
  Lines             ?    44878           
  Branches          ?        0           
=========================================
  Hits              ?    15685           
  Misses            ?    27073           
  Partials          ?     2120

AkihiroSuda · 2018-07-12T06:31:37Z

dockerversion/version_lib.go

Better to have a default value? (moby? moby-engine? dockerd?)

I think it's better to keep it empty, because there are no "moby releases". Docker will set this.

tonistiigi · 2018-07-12T18:46:25Z

cmd/dockerd/docker.go

if dockerversion.ProductName != ""

thaJeztah

minor nits and question, but LGTM otherwise; let me know if you want to address them in this PR, or want to do so as a follow up

thaJeztah · 2018-07-12T08:28:48Z

hack/make/.go-autogen

"${PRODUCT}" ?

thaJeztah · 2018-07-16T10:13:11Z

builder/builder-next/controller.go

Why's this commented out? Does this need a TODO?

thaJeztah · 2018-07-16T10:15:51Z

builder/builder-next/exporter/export.go

Not a super-fan of this boolean argument, but 🤷‍♂️

That's in buildkit. Feel free to change it there, but I don't think it's a blocker for this PR.

Yes, I saw it was implemented there :-)

Signed-off-by: Tibor Vass <[email protected]>

This introduces a PRODUCT environment variable that is used to set a constant at dockerversion.ProductName. That is then used to set BuildKit's ExportedProduct variable in order to show useful error messages to users when a certain version of the product doesn't support a BuildKit feature. Signed-off-by: Tibor Vass <[email protected]>

Signed-off-by: Tibor Vass <[email protected]>

tiborvass · 2018-07-16T21:43:52Z

@thaJeztah addressed.

thaJeztah

LGTM

tiborvass · 2018-07-17T19:09:37Z

unrelated CI failures

tiborvass requested a review from tonistiigi as a code owner July 11, 2018 22:17

GordonTheTurtle added the status/0-triage label Jul 11, 2018

thaJeztah added area/builder Build status/2-code-review and removed status/0-triage labels Jul 11, 2018

thaJeztah reviewed Jul 11, 2018

View reviewed changes

tiborvass mentioned this pull request Jul 11, 2018

[18.06] Set BuildKit's ExportedProduct variable to show useful errors in the future docker-archive/engine#21

Merged

tonistiigi approved these changes Jul 11, 2018

View reviewed changes

tiborvass force-pushed the vendor-buildkit branch from f30077a to 0e57b2d Compare July 12, 2018 02:00

tiborvass requested a review from tianon as a code owner July 12, 2018 02:00

tiborvass changed the title ~~vendor: buildkit to c3846bd8c419d280053e4a9ba4506577665ab94d~~ Set BuildKit's ExportedProduct variable to show useful errors in the future Jul 12, 2018

tiborvass requested a review from tonistiigi July 12, 2018 02:02

tiborvass force-pushed the vendor-buildkit branch from 0e57b2d to 1aa26e2 Compare July 12, 2018 02:08

AkihiroSuda reviewed Jul 12, 2018

View reviewed changes

AkihiroSuda added the rebuild/janky label Jul 12, 2018

GordonTheTurtle removed the rebuild/janky label Jul 12, 2018

tonistiigi reviewed Jul 12, 2018

View reviewed changes

cmd/dockerd/docker.go Outdated

Copy link
Copy Markdown

Member

tonistiigi Jul 12, 2018

Choose a reason for hiding this comment

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

if dockerversion.ProductName != ""

tiborvass force-pushed the vendor-buildkit branch from 1aa26e2 to 5080d67 Compare July 13, 2018 20:16

thaJeztah requested changes Jul 16, 2018

View reviewed changes

Tibor Vass added 3 commits July 16, 2018 21:41

vendor: buildkit to 98f1604

0ab7c1c

Signed-off-by: Tibor Vass <[email protected]>

validate: please vet

8159922

Signed-off-by: Tibor Vass <[email protected]>

tiborvass force-pushed the vendor-buildkit branch from 5080d67 to 8159922 Compare July 16, 2018 21:42

thaJeztah approved these changes Jul 16, 2018

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Jul 16, 2018

vdemeester added the rebuild/janky label Jul 17, 2018

vdemeester added rebuild/experimental labels Jul 17, 2018

GordonTheTurtle removed rebuild/experimental labels Jul 17, 2018

tiborvass merged commit 9ebed53 into moby:master Jul 17, 2018

Conversation

tiborvass commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Jul 16, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Jul 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tiborvass commented Jul 11, 2018 •

edited

Loading

codecov bot commented Jul 12, 2018 •

edited

Loading