-
Notifications
You must be signed in to change notification settings - Fork 509
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
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
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)
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.
Can we add a test for this, making sure a new unknown key doesn't cause a validation error.
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.
@tonistiigi Done
Signed-off-by: CrazyMax <[email protected]>
861f42a
to
07db2be
Compare
fixes #2853
We should validate buildkit configuration before creating the builder.