Skip to content

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Sep 22, 2020

depends on #2842 merged
fixes #2680
implements #1427 (comment)

With this patch, the --progress, --secret, --ssh, and --output flags
trigger an error when trying to use without BuildKit enabled;

DOCKER_BUILDKIT=0 docker build --progress=plain .
--progress is only supported with BuildKit enabled. Enable BuildKit with DOCKER_BUILDKIT=1

DOCKER_BUILDKIT=0 docker build --output=foo .
--output is only supported with BuildKit enabled. Enable BuildKit with DOCKER_BUILDKIT=1

Likewise, options that are not supported yet by BuildKit, now trigger an error:

DOCKER_BUILDKIT=1 docker build --memory=500M .
--memory is not supported with BuildKit enabled. Disable BuildKit with DOCKER_BUILDKIT=0

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member Author

@thaJeztah thaJeztah changed the title build: print error if BuildKit-specific flags are used without BuildKit enabled build: print error if BuildKit/non-BuildKit-specific flags are used Sep 22, 2020
@thaJeztah thaJeztah changed the title build: print error if BuildKit/non-BuildKit-specific flags are used [WIP] build: print error if BuildKit/non-BuildKit-specific flags are used Sep 22, 2020
@thaJeztah
Copy link
Member Author

Pushed a second commit to try integrating it with areFlagsSupported() however, that function is called by isSupported(), which is called by PersistentPreRunE, but I think we override that for build; checking now

@codecov-commenter
Copy link

Codecov Report

Merging #2736 into master will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2736      +/-   ##
==========================================
- Coverage   57.15%   57.13%   -0.02%     
==========================================
  Files         297      297              
  Lines       18657    18663       +6     
==========================================
  Hits        10663    10663              
- Misses       7132     7136       +4     
- Partials      862      864       +2     

@thaJeztah
Copy link
Member Author

opened #2737 to remove the pre-run hack

silvin-lubecki
silvin-lubecki previously approved these changes Sep 22, 2020
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@thaJeztah
Copy link
Member Author

Squashed the commits, and rebased on top of #2737 (that one needs to be merged first)

@silvin-lubecki
Copy link
Contributor

@thaJeztah this PR needs a rebase 🦁

With this patch, the `--progress`, `--secret`, `--ssh`, and `--output` flags
trigger an error when trying to use without BuildKit enabled;

    DOCKER_BUILDKIT=0 docker build --progress=plain .
    --progress is only supported with BuildKit enabled. Enable BuildKit with DOCKER_BUILDKIT=1

    DOCKER_BUILDKIT=0 docker build --output=foo .
    --output is only supported with BuildKit enabled. Enable BuildKit with DOCKER_BUILDKIT=1

Likewise, options that are not supported yet by BuildKit, now trigger an error:

    DOCKER_BUILDKIT=1 docker build --memory=500M .
    --memory is not supported with BuildKit enabled. Disable BuildKit with DOCKER_BUILDKIT=0

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah changed the title [WIP] build: print error if BuildKit/non-BuildKit-specific flags are used build: print error if BuildKit/non-BuildKit-specific flags are used Nov 17, 2020
@thaJeztah
Copy link
Member Author

@tiborvass PTAL

@tonistiigi
Copy link
Member

The flags marked with no-buildkit annotation do not change the outcome of the build, with the exception of security-opt maybe. So I'm not in favor of erroring on them. Hiding should be enough to recommend not using them.

@thaJeztah
Copy link
Member Author

Do you think we should print a warning? e.g. if someone expects to have set memory limits, but they are not applied?

@codecov-io
Copy link

Codecov Report

Merging #2736 (e2986f4) into master (51a0914) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2736      +/-   ##
==========================================
- Coverage   57.10%   57.08%   -0.02%     
==========================================
  Files         297      297              
  Lines       18635    18641       +6     
==========================================
  Hits        10642    10642              
- Misses       7134     7138       +4     
- Partials      859      861       +2     

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.

Docker build output tar file not working

6 participants