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.

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

Set BuildKit's ExportedProduct variable to show useful errors in the future #37439

Merged
merged 3 commits into from Jul 17, 2018

Conversation

Projects
None yet
6 participants
@tiborvass
Copy link
Collaborator

tiborvass commented Jul 11, 2018

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 c3846bd8c419d280053e4a9ba4506577665ab94d

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jul 11, 2018

github.com/containerd/containerd 08f7ee9828af1783dc98cc5cc1739e915697c667
github.com/containerd/typeurl f6943554a7e7e88b3c14aad190bf05932da84788
github.com/containerd/containerd b41633746ed4833f52c3c071e8edcfa2713e5677
github.com/containerd/typeurl a93fcdb778cd272c6e9b3028b2f42d813e785d40

This comment has been minimized.

@thaJeztah

thaJeztah Jul 11, 2018

Member

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

@tiborvass tiborvass force-pushed the tiborvass:vendor-buildkit branch from f30077a to 0e57b2d Jul 12, 2018

@tiborvass tiborvass requested a review from tianon as a code owner Jul 12, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 12, 2018

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

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

@tiborvass tiborvass requested a review from tonistiigi Jul 12, 2018

@tiborvass tiborvass force-pushed the tiborvass:vendor-buildkit branch from 0e57b2d to 1aa26e2 Jul 12, 2018

@@ -14,4 +14,5 @@ const (
RuncCommitID = "library-import"
InitCommitID = "library-import"
PlatformName = ""
ProductName = ""

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Jul 12, 2018

Member

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

This comment has been minimized.

@tiborvass

tiborvass Jul 13, 2018

Author Collaborator

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

@@ -42,6 +43,10 @@ func newDaemonCommand() *cobra.Command {
return cmd
}

func init() {
apicaps.ExportedProduct = dockerversion.ProductName

This comment has been minimized.

@tonistiigi

tonistiigi Jul 12, 2018

Member

if dockerversion.ProductName != ""

@tiborvass tiborvass force-pushed the tiborvass:vendor-buildkit branch from 1aa26e2 to 5080d67 Jul 13, 2018

@thaJeztah
Copy link
Member

thaJeztah left a comment

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

@@ -21,6 +21,7 @@ const (
IAmStatic string = "${IAMSTATIC:-true}"
ContainerdCommitID string = "${CONTAINERD_COMMIT}"
PlatformName string = "${PLATFORM}"
ProductName string = "$PRODUCT"

This comment has been minimized.

@thaJeztah

thaJeztah Jul 16, 2018

Member

"${PRODUCT}" ?

Frontends: frontends,
CacheKeyStorage: cacheStorage,
ResolveCacheImporterFunc: registryremotecache.ResolveCacheImporterFunc(opt.SessionManager),
// ResolveCacheExporterFunc: ce,

This comment has been minimized.

@thaJeztah

thaJeztah Jul 16, 2018

Member

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

@@ -83,7 +83,7 @@ func (e *imageExporterInstance) Export(ctx context.Context, ref cache.ImmutableR
if ref != nil {
layersDone := oneOffProgress(ctx, "exporting layers")

if err := ref.Finalize(ctx); err != nil {
if err := ref.Finalize(ctx, true); err != nil {

This comment has been minimized.

@thaJeztah

thaJeztah Jul 16, 2018

Member

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

This comment has been minimized.

@tiborvass

tiborvass Jul 16, 2018

Author Collaborator

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

This comment has been minimized.

@thaJeztah

thaJeztah Jul 16, 2018

Member

Yes, I saw it was implemented there :-)

tiborvass added some commits Jul 11, 2018

vendor: buildkit to 98f1604134f945d48538ffca0e18662337b4a850
Signed-off-by: Tibor Vass <tibor@docker.com>
builder: set buildkit's exported product variable via PRODUCT
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 <tibor@docker.com>
validate: please vet
Signed-off-by: Tibor Vass <tibor@docker.com>

@tiborvass tiborvass force-pushed the tiborvass:vendor-buildkit branch from 5080d67 to 8159922 Jul 16, 2018

@tiborvass

This comment has been minimized.

Copy link
Collaborator Author

tiborvass commented Jul 16, 2018

@thaJeztah addressed.

@thaJeztah
Copy link
Member

thaJeztah left a comment

LGTM

@tiborvass

This comment has been minimized.

Copy link
Collaborator Author

tiborvass commented Jul 17, 2018

unrelated CI failures

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

4 of 7 checks passed

experimental Jenkins build Docker-PRs-experimental 41360 has failed
Details
powerpc Jenkins build Docker-PRs-powerpc 10552 has failed
Details
z Jenkins build Docker-PRs-s390x 10433 has failed
Details
dco-signed All commits are signed
janky Jenkins build Docker-PRs 50130 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 4406 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 21459 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.