Skip to content
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

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Conversation

crazy-max
Copy link
Member

We can't use the buildkitd config from cmd so move it to util. 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 the docker-container driver.

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

Copy link
Member

@tonistiigi tonistiigi left a 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.

@crazy-max crazy-max force-pushed the move-config branch 2 times, most recently from feb42c8 to 3b64d69 Compare September 14, 2021 09:42
@crazy-max
Copy link
Member Author

A package that contains the buildkit specific configuration structures should not be in util

Ok I thought it was fine like appcontext and appdefaults.

I'd at least hope that we can leave the containerd/userns dependency out of here.

Moved to cmd/buildkit so now dep is removed in appconfig.

@tonistiigi
Copy link
Member

tonistiigi commented Sep 14, 2021

appcontext is not buildkit specific. It is used by other projects and potentially helpful for any cli tool.

appdefaults correct, shouldn't be under util as well. Or the buildkit specifics should be passed as parameters.

@crazy-max
Copy link
Member Author

@tonistiigi Ok now toml parsing is in its own pkg and moved back config to cmd/buildkitd/config.

util/appconfig/load.go Outdated Show resolved Hide resolved
@crazy-max crazy-max force-pushed the move-config branch 3 times, most recently from c2f20c4 to 437da63 Compare September 16, 2021 16:51
@crazy-max
Copy link
Member Author

@tonistiigi Moved to ./config. Also moved the resolver config in ./config so we don't depend on the resolver if we only want to use the config.

@tonistiigi
Copy link
Member

  • avoid creating new top level packages if there isn't a clear need for them. Config is for buildkitd, there is no buildkit-config.
  • everything under util shouldn't be implementing buildkit-only feature logic. It should not generally import anything other than vendor and other util packages.
  • under cmd/buildkitd you can split current logic into multiple packages as you wish

@crazy-max crazy-max changed the title Move config to a dedicated pkg Move config parsing to a dedicated pkg Sep 17, 2021
@crazy-max crazy-max force-pushed the move-config branch 2 times, most recently from 2ab4bc6 to da188c1 Compare September 17, 2021 10:07
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Copy link
Member

@tonistiigi tonistiigi left a 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.

@tonistiigi tonistiigi merged commit 7fb8e74 into moby:master Sep 20, 2021
@crazy-max crazy-max deleted the move-config branch September 20, 2021 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants