-
Notifications
You must be signed in to change notification settings - Fork 495
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
builder: validate buildkit configuration #2864
base: master
Are you sure you want to change the base?
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.
🎉
$ docker buildx create --buildkitd-config cfg.toml
ERROR: failed to parse buildkit config: (2, 1): Can't convert maybe(string) to bool
Signed-off-by: CrazyMax <[email protected]>
ac887c6
to
861f42a
Compare
@@ -151,7 +152,11 @@ func LoadConfigTree(fp string) (*toml.Tree, error) { | |||
defer f.Close() | |||
t, err := toml.LoadReader(f) | |||
if err != nil { | |||
return t, errors.Wrap(err, "failed to parse config") | |||
return t, errors.Wrap(err, "failed to parse buildkit config") | |||
} |
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.
Wondering; will this be problematic when crating a builder using an older version of buildkit? (i.e. would validation only work correctly if the version matches the one that's vendored?)
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 don't think this will be an issue as we are backward compat. Also we are already using this logic to get the registry config in
buildx/store/storeutil/storeutil.go
Lines 125 to 128 in 567361d
config, err := buildkitdconfig.Load(bytes.NewReader(dt)) | |
if err != nil { | |
return opt, err | |
} |
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.
Ah yeah, I wasn't sure, so thought I'd mention it. I recall that I ran into changes in the garbage-collect configuration that changed, and for which I needed to make changes in moby/moby, because it used those; moby/moby#48634 (comment)
fixes #2853
We should validate buildkit configuration before creating the builder.