Skip to content

load: move env var profile detection to option#446

Merged
milas merged 1 commit intocompose-spec:masterfrom
milas:profile-env-var
Aug 29, 2023
Merged

load: move env var profile detection to option#446
milas merged 1 commit intocompose-spec:masterfrom
milas:profile-env-var

Conversation

@milas
Copy link
Copy Markdown
Member

@milas milas commented Aug 21, 2023

Instead of reading COMPOSE_PROFILES environment variable directly during load(), rely exclusively on opts.Profiles.

A new loader option, WithDefaultProfiles, allows passing profile name(s) via varargs. If none are provided, it will fallback to using any profiles set via COMPOSE_PROFILES.

This improves purity of the loader and fixes issues with profiles being ignored for include (docker/compose#10906).

@milas milas added the bug Something isn't working label Aug 21, 2023
@milas milas requested review from glours, laurazard and ndeloof August 21, 2023 19:32
@milas milas self-assigned this Aug 21, 2023
Instead of reading `COMPOSE_PROFILES` environment variable directly
during `load()`, rely exclusively on `opts.Profiles`.

A new loader option, `WithDefaultProfiles`, allows passing profile
name(s) via varargs. If none are provided, it will fallback to using
any profiles set via `COMPOSE_PROFILES`.

This improves purity of the loader and fixes issues with profiles
being ignored for `include` (docker/compose#10906).

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
@milas
Copy link
Copy Markdown
Member Author

milas commented Aug 21, 2023

Also, technically COMPOSE_PROFILES is not part of the spec (we don't specify HOW things like CLI should work).

At the same time, one thing we've learned from docker/cli is that it's really annoying if a bunch of conventions (DOCKER_CONTEXT) aren't supported by the "lower-level" client (moby/moby). So that's why I went with the WithDefaultProfiles loader option here - it keeps the logic in compose-go for the broader ecosystem, but it's also easy to avoid by using WithProfiles instead.

@milas
Copy link
Copy Markdown
Member Author

milas commented Aug 25, 2023

@ndeloof This is slightly redundant now with your fix from #448 but I think it still makes sense to remove os.GetEnv logic from the loader itself, wdyt? Otherwise, I can close it

@ndeloof
Copy link
Copy Markdown
Collaborator

ndeloof commented Aug 25, 2023

@milas loader definitively should not use os.GetEnv

@milas milas merged commit 8c4b79e into compose-spec:master Aug 29, 2023
@milas milas deleted the profile-env-var branch August 29, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants