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

image/build: print warning/error for unsupported buildkit options #1896

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zyqsempai
Copy link

@Zyqsempai Zyqsempai commented May 21, 2019

Based on: #1755

Added error return on squash command and warnings for all the rest, unsupported by buildkit options.

Signed-off-by: Boris Popovschi [email protected]

@AkihiroSuda

@codecov-io
Copy link

Codecov Report

Merging #1896 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1896      +/-   ##
==========================================
- Coverage   56.75%   56.71%   -0.05%     
==========================================
  Files         309      309              
  Lines       21680    21696      +16     
==========================================
  Hits        12305    12305              
- Misses       8476     8492      +16     
  Partials      899      899

@@ -71,6 +73,32 @@ func runBuildBuildKit(dockerCli command.Cli, options buildOptions) error {

stdoutUsed := false

if options.squash {
// return error rather than print a warning, as squash is often used for hiding secrets
return errors.New("squash is not supported by BuildKit. Consider using a multi-stage Dockerfile instead")
Copy link
Collaborator

Choose a reason for hiding this comment

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

supported but slightly broken
moby/moby#38903

Copy link
Author

Choose a reason for hiding this comment

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

So what should we output in error message?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a PR moby/moby#39187 , probably we can just remove the error here

cc @tonistiigi

Copy link
Author

Choose a reason for hiding this comment

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

Any news on this one?

Copy link
Member

Choose a reason for hiding this comment

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

ping @tonistiigi ptal

@thaJeztah
Copy link
Member

I'm a bit on the fence on this one, as I'd prefer the daemon to return these errors/warnings, given that a client could be used against different versions of the daemon

@Zyqsempai
Copy link
Author

Hey, guys, what is the status of this one?

@Zyqsempai
Copy link
Author

@thaJeztah any news on this one?

@thaJeztah
Copy link
Member

Still not sure about this; we should annotate them with the no-buildkit annotation though, so that they're hidden if buildkit is not enabled. That way we can also use that information to label them in the reference docs (as we do for minimum API version and experimental);

Screenshot 2019-10-04 at 15 49 04

Let me push a quick PR with those changes; we can continue the discussion on this one (but I'd still prefer the daemon to be the source of truth here)

@thaJeztah
Copy link
Member

Opened #2123

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

Successfully merging this pull request may close these issues.

6 participants