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

bake: load .env file from working dir for compose files #1261

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Aug 9, 2022

fixes #282
carry #489

Loads .env file if available in the working directory for compose files. The file format is the same as the compose library: https://github.com/compose-spec/compose-go/tree/master/dotenv.

bake/bake.go Outdated Show resolved Hide resolved
bake/bake.go Show resolved Hide resolved
bake/env.go Outdated Show resolved Hide resolved
bake/env.go Outdated Show resolved Hide resolved
bake/env.go Outdated Show resolved Hide resolved
bake/env_test.go Outdated Show resolved Hide resolved
commands/bake.go Outdated Show resolved Hide resolved
@crazy-max crazy-max changed the title bake: read env vars from alternate env-file bake: read env vars from file Aug 9, 2022
@crazy-max crazy-max force-pushed the bake-env branch 3 times, most recently from a64873c to 01bb0a8 Compare August 9, 2022 14:51
@crazy-max crazy-max requested a review from jedevc August 9, 2022 14:56
@crazy-max crazy-max added this to the v0.9.0 milestone Aug 9, 2022
@crazy-max crazy-max marked this pull request as draft August 9, 2022 17:34
@crazy-max
Copy link
Member Author

As discussed with @tonistiigi I changed the current behavior (see updated PR description).

@crazy-max crazy-max marked this pull request as ready for review August 9, 2022 21:31
bake/bake.go Outdated
Comment on lines 223 to 224
default:
e, err := dotenv.UnmarshalBytes(f.Data)
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, files with an unknown extension were parsed using the HCL parser. Now they will be parsed as env files. @tonistiigi Do you think it could be an issue? Maybe instead we could check for .env extension and default to hcl?

Copy link
Member

Choose a reason for hiding this comment

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

Previously, files with an unknown extension were parsed using the HCL parser. Now they will be parsed as env files.

What is the difference?

I would prefer to not change the -f behavior and only handle .env as a special case when parsing compose.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think I understand you intentions better now.

Still in the first PR maybe .env is enough. Just to have compatibility.

For the -f we can maybe first split the files between compose and HCL. Then parse HCL and store all the attributes in the process. Then parse compose(for the second time if needed) and pass the attributes and variables.

If we support -f this way it shouldn't be only for some special limited set of HCL files that don't have any blocks but ideally all the vars that would be replaced in HCL would be replaced in Compose yaml as well(and additionally only Compose does .env as well).

Copy link
Member Author

Choose a reason for hiding this comment

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

For the -f we can maybe first split the files between compose and HCL. Then parse HCL and store all the attributes in the process. Then parse compose(for the second time if needed) and pass the attributes and variables.

If we support -f this way it shouldn't be only for some special limited set of HCL files that don't have any blocks but ideally all the vars that would be replaced in HCL would be replaced in Compose yaml as well(and additionally only Compose does .env as well).

Ah right we also need to store the attributes before passing them to HCL/JSON/Compose files when parsed. I missed that.

I will just add .env support for compose in this PR and open another one for -f.

Copy link

@saurabhdes saurabhdes Sep 21, 2022

Choose a reason for hiding this comment

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

@crazy-max Thankyou for this PR to fix load .env file from working dir for compose files ! I was facing the exact issue that's discussed here and it's resolved.
Very much looking forward to add another option -f file to load another .env data file such that the below command will run or please suggest another option to pass a custom env variable file. Essentially same as #1135

docker buildx bake -f docker-compose.yml -f .ghcr.env should work.

bake/bake.go Outdated Show resolved Hide resolved
@crazy-max crazy-max changed the title bake: read env vars from file bake: load .env file from working dir for compose files Aug 9, 2022
bake/compose.go Outdated
func parseCompose(dt []byte) (*compose.Project, error) {
return loader.Load(compose.ConfigDetails{
func ParseCompose(dt []byte, envs map[string]string) (*Config, error) {
if wd, err := os.Getwd(); err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this suits better in ParseComposeFile. envs is also passed to this function so shouldn't be extended. But ParseComposeFile already reads env from the host.

return curenv
}

dt, err := os.ReadFile(ef)
Copy link
Member

Choose a reason for hiding this comment

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

I think these syscalls should still error unless the error is notexist

bake/compose.go Outdated

envs, err := dotenv.UnmarshalBytes(dt)
if err != nil {
return curenv
Copy link
Member

Choose a reason for hiding this comment

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

Is the behavior to ignore parse error the behavior in compose as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I read the wrong implementation. The right one from compose-go indeed check for errors.

@tonistiigi tonistiigi merged commit 5b2e1d3 into docker:master Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker buildx bake should apply env variable from .env
4 participants