Skip to content

[fix] up should not silently ignore missing depends_on service#10602

Merged
ndeloof merged 1 commit intodocker:v2from
ndeloof:up_depends_on
May 26, 2023
Merged

[fix] up should not silently ignore missing depends_on service#10602
ndeloof merged 1 commit intodocker:v2from
ndeloof:up_depends_on

Conversation

@ndeloof
Copy link
Copy Markdown
Contributor

@ndeloof ndeloof commented May 23, 2023 •
edited
Loading

What I did
docker compose up used to silently ignore any dependency service that is missing in the project model as disabled service. This fix detect this and report a diagnostic message to the user

$ docker compose up
[+] Building 0.0s (0/0)                                                                                                                        
service mssql is required by app but is disabled. Can be enabled by profiles [dev]

Related issue
fixes #9575

require compose-spec/compose-go#409

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

@ndeloof ndeloof requested review from a team, StefanScherer, glours, laurazard, milas, nicksieger and ulyssessouza and removed request for a team May 23, 2023 12:23
@codecov
Copy link
Copy Markdown

codecov bot commented May 23, 2023 •
edited
Loading

Codecov Report

Patch coverage: 47.61% and project coverage change: -0.10 ⚠️

Comparison is base (c7afc61) 59.43% compared to head (281ffd0) 59.34%.

Additional details and impacted files 
@@            Coverage Diff             @@
##               v2   #10602      +/-   ##
==========================================
- Coverage   59.43%   59.34%   -0.10%     
==========================================
  Files         107      107              
  Lines        9437     9395      -42     
==========================================
- Hits         5609     5575      -34     
+ Misses       3249     3244       -5     
+ Partials      579      576       -3
Impacted Files Coverage Δ
pkg/compose/dependencies.go 84.66% <40.00%> (-3.24%) ⬇️
pkg/compose/create.go 61.63% <66.66%> (+0.53%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

glours
glours approved these changes May 23, 2023
View reviewed changes
Copy link
Copy Markdown
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.

👍

@glours
Copy link
Copy Markdown
Contributor

glours commented May 23, 2023

You need to update some tests:

--- FAIL: TestStartStopWithDependencies/Up_no-deps_links (1.24s)
        start_stop_test.go:118: Running command: docker compose --project-name e2e-start-stop-with-dependencies down
        start_stop_test.go:119: Running command: docker compose -f ./fixtures/links/compose.yaml --project-name e2e-start-stop-with-dependencies up --no-deps -d foo
        start_stop_test.go:119: assertion failed: 
            Command:  docker compose -f ./fixtures/links/compose.yaml --project-name e2e-start-stop-with-dependencies up --no-deps -d foo
            ExitCode: 1
            Error:    exit status 1
            Stdout:   
            Stderr:   service bar is required by foo but is disabled. Can be enabled by profiles []
            
            
            Failures:
            ExitCode was 1 expected 0
            Expected no error
    start_stop_test.go:128: Running command: docker compose --project-name e2e-start-stop-with-dependencies rm -fsv

@ndeloof
Copy link
Copy Markdown
Contributor Author

ndeloof commented May 23, 2023

this is not just about fixing a test, --no-deps makes it explicit we don't want this error to be a blocker. Will need to revisit my fix

@glours
Copy link
Copy Markdown
Contributor

glours commented May 23, 2023

Arff 😬
Another example why tests are useful 😂

@ndeloof ndeloof mentioned this pull request May 23, 2023
Merged
@ndeloof ndeloof mentioned this pull request May 24, 2023
Merged
@ndeloof ndeloof force-pushed the up_depends_on branch 2 times, most recently from 56413ac to 247d2fd Compare May 24, 2023 16:22
@ndeloof
Copy link
Copy Markdown
Contributor Author

ndeloof commented May 24, 2023

waiting for compose-spec/compose-go#409 so we can merge

@ndeloof
up should not silently ignore missing depends_on service
281ffd0 
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof merged commit 06ec064 into docker:v2 May 26, 2023
@ndeloof ndeloof deleted the up_depends_on branch May 26, 2023 19:59
@kdrag0n kdrag0n mentioned this pull request Jun 27, 2023
Closed
@nmguse-bighealth nmguse-bighealth mentioned this pull request Jun 28, 2023
Closed
@allanlewis
Copy link
Copy Markdown

allanlewis commented Jun 29, 2023

This change appears to have caused #10751, which is breaking my CI workflows. I'm working around it by downgrading to v2.18.1.

@alexintech alexintech mentioned this pull request Jul 11, 2023
Closed
@kiblik kiblik mentioned this pull request Jul 19, 2023
Closed
@acran acran mentioned this pull request Sep 2, 2023
Closed
@Vigilans Vigilans mentioned this pull request Apr 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glours glours glours approved these changes

@nicksieger nicksieger Awaiting requested review from nicksieger nicksieger was automatically assigned from docker/compose-maintainers

@StefanScherer StefanScherer Awaiting requested review from StefanScherer StefanScherer was automatically assigned from docker/compose-maintainers

@ulyssessouza ulyssessouza Awaiting requested review from ulyssessouza ulyssessouza was automatically assigned from docker/compose-maintainers

@laurazard laurazard Awaiting requested review from laurazard laurazard was automatically assigned from docker/compose-maintainers

+1 more reviewer

@milas milas milas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

docker compose start/stop/restart fails with depending container disabled via profiles

4 participants

@ndeloof @glours @allanlewis @milas