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

only load env_file after services have been selected #12611

Merged
merged 1 commit into from
Mar 10, 2025

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Mar 6, 2025

What I did
resolve env_file after active services have been selected, so we don't read (potentially missing) env_file for inactive ones

Related issue
fixes #8713
requires compose-spec/compose-go#751

(not mandatory) A picture of a cute animal, if possible in relation to what you did

@ndeloof ndeloof requested a review from a team as a code owner March 6, 2025 15:18
@ndeloof ndeloof requested review from glours and Copilot and removed request for glours March 6, 2025 15:18

Choose a reason for hiding this comment

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

PR Overview

This PR ensures that env_file entries are only loaded for active services, preventing potential errors when an env_file does not exist for inactive services.

  • Adds a new compose file fixture with two services where only one references an env_file.
  • Introduces a new end-to-end test to confirm that missing env_file for inactive services does not trigger an error.
  • Adjusts the project environment resolution logic in cmd/compose/compose.go to resolve env_file only after services selection.

Reviewed Changes

File Description
pkg/e2e/fixtures/env_file/compose.yaml New test fixture with two services, one specifying a missing env_file
pkg/e2e/env_file_test.go Added test to verify that env_file for inactive services is not loaded
cmd/compose/compose.go Updated environment resolution order to resolve after services are selected

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

pkg/e2e/env_file_test.go:38

  • Consider adding an assertion to verify that the missing env_file for serviceB is not processed, ensuring the test precisely validates that inactive services do not trigger an env_file load error.
    c.RunDockerComposeCmd(t, "-f", "./fixtures/env_file/compose.yaml", "up", "-d", "serviceA")
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ndeloof ndeloof force-pushed the UnusedMissingEnvFile branch from 35f3e64 to 6297e75 Compare March 9, 2025 15:05
@ndeloof ndeloof enabled auto-merge (rebase) March 9, 2025 15:05
@ndeloof ndeloof force-pushed the UnusedMissingEnvFile branch from 6297e75 to de64016 Compare March 9, 2025 15:08
@ndeloof ndeloof force-pushed the UnusedMissingEnvFile branch from de64016 to 2bd65f4 Compare March 10, 2025 07:21
@ndeloof ndeloof merged commit b237289 into docker:main Mar 10, 2025
25 checks passed
@ndeloof ndeloof deleted the UnusedMissingEnvFile branch March 10, 2025 08:14
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.

.env file referenced underneath env_file in a compose file is being parsed when file is not used.
2 participants