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

Remove "experimental" annotations for buildkit #1303

Merged

Conversation

thaJeztah
Copy link
Member

BuildKit can now be enabled without the daemon having experimental features enabled.

related engine PR; moby/moby#37686

BuildKit can now be enabled without the daemon having
experimental features enabled.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

ping @andrewhsu @tiborvass @AntaresS PTAL


flags.StringArrayVar(&options.secrets, "secret", []string{}, "Secret file to expose to the build (only if BuildKit enabled): id=mysecret,src=/local/secret")
flags.SetAnnotation("secret", "experimental", nil)
Copy link
Member Author

Choose a reason for hiding this comment

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

Wonder if we should have a new annotation to hide / disable these if the CLI (or daemon) is configured to use builder v1

Copy link
Contributor

Choose a reason for hiding this comment

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

you mean hide the progress and secret flags?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably better when builder v1 is configured on the cli side

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes; not a blocker for now, but would be good to look into

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@codecov-io
Copy link

codecov-io commented Aug 21, 2018

Codecov Report

Merging #1303 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1303      +/-   ##
==========================================
- Coverage   54.77%   54.76%   -0.02%     
==========================================
  Files         292      292              
  Lines       19285    19285              
==========================================
- Hits        10563    10561       -2     
- Misses       8063     8064       +1     
- Partials      659      660       +1

@thaJeztah
Copy link
Member Author

Pushed a second commit to move "sessions" out of experimental as well (per moby/moby#37686)

Copy link
Collaborator

@tiborvass tiborvass 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

@tiborvass should the --stream option also be moved out of experimental, or is that separate?

flags.BoolVar(&options.stream, "stream", false, "Stream attaches to server to negotiate build context")
flags.SetAnnotation("stream", "experimental", nil)

Copy link
Contributor

@AntaresS AntaresS left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -28,6 +28,9 @@ import (
const clientSessionRemote = "client-session"

func isSessionSupported(dockerCli command.Cli) bool {
if versions.GreaterThanOrEqualTo(dockerCli.Client().ClientVersion(), "1.39") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should check daemon version, not CLI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Daemon could be version 1.40, but the client downgraded to 1.10; in that case the API version that's used is 1.10, and it's not supported

Copy link
Collaborator

@tiborvass tiborvass left a comment

Choose a reason for hiding this comment

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

we should check daemon version not cli

Copy link
Collaborator

@tiborvass tiborvass 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 thaJeztah merged commit 08cf36d into docker:master Aug 21, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.09.0 milestone Aug 21, 2018
@thaJeztah thaJeztah deleted the remove_buildkit_experimental_annotations branch August 21, 2018 23:57
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.

5 participants