-
Notifications
You must be signed in to change notification settings - Fork 503
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
create: load default buildkit config if none specified #1111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, I will very much end up using this a lot 🎉
If the configuration file is not specified, will look for one by default in: | ||
* `$BUILDX_CONFIG/buildkitd.default.toml` | ||
* `$DOCKER_CONFIG/buildx/buildkitd.default.toml` | ||
* `~/.docker/buildx/buildkitd.default.toml` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to run without a configuration file at all, if the default one exists? I suppose you'd just point the config file to /dev/null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's the responsibility of the user. Looking at the XDG Base Directory Specification, the only case where config will not be loaded is:
When attempting to read a file, if for any reason a file in a certain directory is unaccessible, e.g. because the directory is non-existent, the file is non-existent or the user is not authorized to open the file, then the processing of the file in that directory should be skipped.
Maybe we could output a message saying that a default configuration has been found in X and is being used to create this builder? Only issue I have is we currently print builder name when it has been created:
Line 228 in 98439f7
fmt.Printf("%s\n", ng.Name) |
Not sure if it was to be able to concat commands like (cc @tonistiigi):
$ docker buildx build --builder $(docker buildx create --use) .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think somehow calling out that we have loaded a default would be useful - I can definitely imagine it being frustrating to create a default, forget about it, and then be confused about why new builders are acting a certain way.
Could we attach messages like this to stderr, which won't affect command substitutions like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to look at adopting XDG in general (where possible). ISTR there's still some "gaps" in the specification, and if we adopt, we must look at (e.g.) macOS
(what's the most logical location? "mac native directories" (~/Library/xxx
) or "Linux(ish)" directories (XDG / ~/.docker
)
Oh! Found it; moby/moby#20693
Question; should this config be "rootless" aware? I recall the pkg/homedir
package in Moby takes that into account; https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/pkg/homedir/homedir_linux.go#L80-L93
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to look at adopting XDG in general
Yeah would like too but atm we rely on docker cli conf path if BUILDX_CONFIG
is not specified:
buildx/util/confutil/config.go
Line 22 in 98439f7
buildxConfig := filepath.Join(filepath.Dir(dockerCli.ConfigFile().Filename), "buildx") |
So I would say it's already supported in https://github.com/docker/cli/blob/master/cli/config/config.go which relies on https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/pkg/homedir/homedir_linux.go#L80-L93.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also instead of using
buildx/util/confutil/config.go
Line 22 in 98439f7
buildxConfig := filepath.Join(filepath.Dir(dockerCli.ConfigFile().Filename), "buildx") |
we could directly use https://github.com/docker/cli/blob/1b922a47c1bb0dda1239a20616e533dfd16d8ac8/cli/config/config.go#L62-L66
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jedevc Added a warning log msg if default config is being used which is streamed to stderr so it will work if called out with another command.
Signed-off-by: CrazyMax <[email protected]>
fixes docker/setup-buildx-action#127
if configuration file is not specified, will look for one by default in:
$BUILDX_CONFIG/buildkitd.default.toml
$DOCKER_CONFIG/buildx/buildkitd.default.toml
~/.docker/buildx/buildkitd.default.toml
this can be useful when a user session should inherit from the same configuration when a builder is created without bothering specifying the config path.
Signed-off-by: CrazyMax [email protected]