Skip to content

vendor: buildkit 4d1f260e8490ec438ab66e08bb105577aca0ce06#2656

Merged
tiborvass merged 1 commit into
docker:masterfrom
thaJeztah:bump_buildkit
Sep 10, 2020
Merged

vendor: buildkit 4d1f260e8490ec438ab66e08bb105577aca0ce06#2656
tiborvass merged 1 commit into
docker:masterfrom
thaJeztah:bump_buildkit

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

full diff: moby/buildkit@df35e98...4d1f260

- Description for the changelog

@thaJeztah

Copy link
Copy Markdown
Member Author

@tonistiigi @tiborvass @AkihiroSuda PTAL if the changes for secrets and git require documentation changes and if local changes are needed to expose the functionality

@thaJeztah

Copy link
Copy Markdown
Member Author

Ah, guess this answers some of my questions;

#14 0.375 Building statically linked build/docker-linux-amd64
#14 26.34 # github.com/docker/cli/cli/command/image
#14 26.34 cli/command/image/build_buildkit.go:427:15: undefined: secretsprovider.FileSource
#14 26.34 cli/command/image/build_buildkit.go:435:16: undefined: secretsprovider.NewFileStore
#14 26.34 cli/command/image/build_buildkit.go:442:34: undefined: secretsprovider.FileSource
#14 26.34 cli/command/image/build_buildkit.go:449:8: undefined: secretsprovider.FileSource
#14 ERROR: executor failed running [/bin/sh -c ./scripts/build/binary]: runc did not terminate sucessfully
@codecov-commenter

codecov-commenter commented Jul 28, 2020

Copy link
Copy Markdown

Codecov Report

Merging #2656 into master will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2656      +/-   ##
==========================================
+ Coverage   58.54%   58.55%   +0.01%     
==========================================
  Files         296      296              
  Lines       21286    21293       +7     
==========================================
+ Hits        12462    12469       +7     
  Misses       7915     7915              
  Partials      909      909              
@tonistiigi

Copy link
Copy Markdown
Member

Spotted possible issue, fixed in moby/buildkit#1613 . Don't need to be vendored but just same fix can be done in cli.

@thaJeztah

Copy link
Copy Markdown
Member Author

I'm a bit confused by the UX. Looking at it, it doesn't seem consistent.

The --secret flag currently supports these keys: type, id, source, src, env

  • id is a required option
  • type is full ignored, unless an invalid type is specified (anything other than "file" or "env" (the code doesn't follow different paths if file or env is specified)
  • both source, src and env can be specified at the same time.
    • this is ambiguous if no explicit "type" is set
  • if type is set to "env", source / src acts as an alias for env (if unset)
  • if only id is set (and no source / src or env), and an environment variable is found that matches that name, the default is now switched to "take value from the environment variable"
    • This is a breaking change: previously id would be used as filename

I think

  • if no type is set, and no source / src / env, use the current default and use id as source (current behavior)
  • if no type is set, but source / src is set; implicitly assume type=file (could be seen as same as above, as file is default)
  • if no type is set, but env is set; implicitly assume type=env
  • if type=file
    • only look at source / src (ignore env)
    • fall back to id if not set
    • produce error if env is set? (invalid option for type=file ?
  • if type=env
    • look at env to use as "source"
    • fall back to source / src if not set
    • fall back to id if not set
    • produce error if both env and source / src are set?

However, from the above; does env= provide much value, given that we already have type=env, and that source/src and id already provide the functionality?
Effectively if would only be useful to allow switching between file and env without having to explicitly set type=.

One "feature" could be to explicitly allow env as a "fallback", so;

--secret id=foo-token,src=~/my-token.txt,env=FOO_TOKEN

Which would either pick the value from $FOO_TOKEN or from ~/my-token.txt (not sure in which order though)

If we would remove env=, the combinations would be reduced quite a bit;

  • if type=file or if no type is set (default)
    • look at source / src
    • fall back to id if not set
    • produce error if the file was not found
  • if type=env
    • look at source / src
    • fall back to id if not set
    • produce error if no environment variable with the given name was found? (TBD)
Comment thread cli/command/image/build_buildkit.go Outdated
return nil, errors.Errorf("unexpected key '%s' in '%s'", key, field)
}
}
if typ == "env" {

@thaJeztah thaJeztah Aug 13, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

need to add this

Suggested change
if typ == "env" {
if typ == "env" && fs.Env == "" {
full diff: moby/buildkit@df35e98...4d1f260

- moby/buildkit#1551 session: track sessions with a group construct
- moby/buildkit#1534 secrets: allow providing secrets with env
- moby/buildkit#1533 git: support for token authentication
- moby/buildkit#1549 progressui: fix logs time formatting

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment