add support of depends_on.required attribute#10792
Conversation
glours
force-pushed
the
add-depends_on-required
branch
from
c70bb66 to
679eec6
Compare
glours
force-pushed
the
add-depends_on-required
branch
from
679eec6 to
f8aa456
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## v2 #10792 +/- ##
==========================================
+ Coverage 59.25% 59.44% +0.19%
==========================================
Files 115 115
Lines 9869 9896 +27
==========================================
+ Hits 5848 5883 +35
+ Misses 3430 3425 -5
+ Partials 591 588 -3
☔ View full report in Codecov by Sentry. |
glours
force-pushed
the
add-depends_on-required
branch
2 times, most recently
from
9b2a58f to
8537832
Compare
glours
force-pushed
the
add-depends_on-required
branch
2 times, most recently
from
518fdac to
09cebc9
Compare
glours
force-pushed
the
add-depends_on-required
branch
from
09cebc9 to
7cc466a
Compare
glours
force-pushed
the
add-depends_on-required
branch
from
7cc466a to
e4671e5
Compare
milas
left a comment
milas
left a comment
There was a problem hiding this comment.
Overall LGTM. I realize I missed the boat on compose-spec/compose-spec#382 😅 but I do wonder if optional: true would be better semantically?
It's a bit weird to have the default/absence of a boolean field be true IMO 😬
| res := c.RunDockerComposeCmd(t, "-f", "./fixtures/dependencies/deps-not-required.yaml", "--project-name", projectName, | ||
| "up", "-d") | ||
| assert.Assert(t, strings.Contains(res.Combined(), "foo"), res.Combined()) | ||
| assert.Assert(t, !strings.Contains(res.Combined(), "bar"), res.Combined()) |
There was a problem hiding this comment.
From compose-spec/compose-spec#382:
Compose only warns you when the dependency service isn't started or available.
❯ docker compose up
[+] Running 2/0
✔ Network required_default Created 0.0s
✔ Container required-foo-1 Created 0.0s
Attaching to required-foo-1
required-foo-1 | foo
required-foo-1 exited with code 0
Shouldn't there be a warning "foo has optional dependency on bar which was not selected by active profiles" or something?
glours
force-pushed
the
add-depends_on-required
branch
3 times, most recently
from
8041b68 to
4e89b87
Compare
Signed-off-by: Guillaume Lours <[email protected]>
glours
force-pushed
the
add-depends_on-required
branch
from
4e89b87 to
c6efc63
Compare
Signed-off-by: Guillaume Lours <[email protected]>
glours
force-pushed
the
add-depends_on-required
branch
from
c6efc63 to
2d16a05
Compare
milas
left a comment
milas
left a comment
There was a problem hiding this comment.
LGTM!
Just a note: I do think we could do some future CLI work on the output to make startup around dependencies in particular clearer, e.g. I'd still love it if here we could convey "foo launched without bar because not-required profile is not active".
3 participants
What I did
Add support of
depends_on.requiredattributeRelated issue
N/A
(not mandatory) A picture of a cute animal, if possible in relation to what you did
