-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Move config parsing to a dedicated pkg #2361
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.
A package that contains the buildkit specific configuration structures should not be in util. cmd/buildkitd
seems like a correct place. I'm fine with separating packages though. Would be better if toml parsing and config definition is not in same package but with the current split I'd at least hope that we can leave the containerd/userns
dependency out of here.
feb42c8
to
3b64d69
Compare
Ok I thought it was fine like
Moved to |
|
3b64d69
to
681b8e6
Compare
be5c6d5
to
8b22da2
Compare
@tonistiigi Ok now toml parsing is in its own pkg and moved back config to |
c2f20c4
to
437da63
Compare
@tonistiigi Moved to |
|
437da63
to
9cd28ae
Compare
2ab4bc6
to
da188c1
Compare
Signed-off-by: CrazyMax <[email protected]>
da188c1
to
3fd4338
Compare
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.
It's generally better to have a pkg that only defines the config structures and no dependency on toml parsing but we can fix that in follow-up if it comes up.
We can't use the buildkitd config from
cmd
so move it toutil
. The idea will be to be able to load the toml configuration from buildx in a follow-up in order to inject the root ca and user tls certs when we create a builder with thedocker-container
driver.Signed-off-by: CrazyMax [email protected]