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

Allow users to have separate store paths #385

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

errordeveloper
Copy link
Contributor

@errordeveloper errordeveloper commented Sep 8, 2020

  • decouple store path from $DOCKER_CONFIG
  • improve containerised build setup
  • introduce new $BUILDX_CONFIG environment variable

Related to #308

commands/util.go Outdated Show resolved Hide resolved
@errordeveloper
Copy link
Contributor Author

Two UX questions:

  • should new behaviour be an opt-in to begin with, or since buildx is beta, it's reasonable to enable it right away?
  • this change will always log store path, should it not log when default is used?

@errordeveloper errordeveloper force-pushed the fix-308 branch 2 times, most recently from cbf1b82 to 947664b Compare September 8, 2020 11:01
@tonistiigi
Copy link
Member

I'm ok with the BUILDX_CONFIG var but not sure about finding .buildx dir. I consider the config directory structure internal but .buildx should be something that can be potentially committed to git.

I think loading a builder config from a file or having project-specific builders is a valid use case but probably these should be handled separately.

@errordeveloper
Copy link
Contributor Author

@tonistiigi thanks for the review, and I think you are right. I will revisit this ping you once it is ready for review.

@errordeveloper
Copy link
Contributor Author

@tonistiigi please have another look now!

commands/util.go Outdated
// of Docker config file (i.e. `${DOCKER_CONFIG}/buildx`)
func getConfigStorePath(dockerCli command.Cli) string {
if buildxConfig := os.Getenv("BUILDX_CONFIG"); buildxConfig != "" {
logrus.Infof("using config store %q based in \"$BUILDX_CONFIG\" environment variable", buildxConfig)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you change these to Debugf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 👍

- decouple store path from `$DOCKER_CONFIG`
- improve containerised build setup
- introduce new `$BUILDX_CONFIG` environment variable

Signed-off-by: Ilya Dmitrichenko <[email protected]>
@tonistiigi tonistiigi merged commit 0269388 into docker:master Sep 10, 2020
@errordeveloper
Copy link
Contributor Author

errordeveloper commented Sep 10, 2020 via email

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.

2 participants