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.

Sign up for GitHub

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

Jump to bottom

Add support for Docker Compose depends_on long syntax #1718

Merged
merged 5 commits into from
Nov 11, 2023
Merged

Add support for Docker Compose depends_on long syntax #1718

merged 5 commits into from
Nov 11, 2023

Conversation

poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Nov 6, 2023 •
edited
Loading

Closes #888

TODO:

  • Healthchecks added via compose are not picked up/usable -> decide on scope for this PR
  • Docs need to be extended
  • Integration testing

@poikilotherm
feat(compose): add compatibility for dependsOn with compose 2.1+ syntax
da570d7 
fabric8io#888

As of Docker Compose v2.1, there is a new map-based long syntax to express dependencies
of containers among each other that is much more sophisticated than the old list based.

This commit solves the immediate problem described in fabric8io#888, where this new syntax
is simply rejected. The dependsOn concept of DMP is now feed with the map's keys.
Limitation: the wait conditions expressed with the new syntax are not added with this commit.

See also: fabric8io#888
See also: https://github.com/compose-spec/compose-spec/blob/master/spec.md#long-syntax-1
See also: https://docs.docker.com/compose/compose-file/compose-file-v2/#depends_on
@poikilotherm
feat(compose): apply compose svc conditions as wait configurations fa…
08d13b3 
…bric8io#888

Extract the Docker Compose v2.1+ depends_on conditions and apply them
as Docker Maven Plugin waiting configurations.

Limitations: this mapping requires an inversion of the data model in use.
Docker Compose has waiting conditions located at the depending service,
while this plugin puts them on the dependent service/s.
@poikilotherm
test(compose): add tests for depends_on handling fabric8io#888
dfcfb87
@poikilotherm
Copy link
Contributor Author

@rohanKanojia @rhuss I opened this draft PR hoping for some initial feedback.

Also: I added unit tests for this, but there seem to be no integration tests. How is testing done to actually make sure it works in reality?

@rohanKanojia
Copy link
Member

There are some integration tests in it/ folder. Could you please check if you can add an integration test project there?

@codecov Codecov
Copy link

codecov bot commented Nov 6, 2023 •
edited
Loading

Codecov Report

Merging #1718 (321e885) into master (f228f64) will increase coverage by 0.11%.
Report is 3 commits behind head on master.
The diff coverage is 83.58%.

❗ Current head 321e885 differs from pull request most recent head 2e8ef0d. Consider uploading reports for the commit 2e8ef0d to get more accurate results

@@             Coverage Diff              @@
##             master    #1718      +/-   ##
============================================
+ Coverage     65.11%   65.22%   +0.11%     
- Complexity     2253     2273      +20     
============================================
  Files           172      172              
  Lines         10109    10171      +62     
  Branches       1390     1402      +12     
============================================
+ Hits           6582     6634      +52     
- Misses         2977     2984       +7     
- Partials        550      553       +3
Files Coverage Δ
...ig/handler/compose/DockerComposeConfigHandler.java 72.97% <83.33%> (+0.80%) ⬆️
...g/handler/compose/DockerComposeServiceWrapper.java 65.86% <83.67%> (+4.66%) ⬆️

@poikilotherm
Copy link
Contributor Author

There are some integration tests in it/ folder. Could you please check if you can add an integration test project there?

Done. Very simple, but clearly shows it works.

@poikilotherm poikilotherm marked this pull request as ready for review November 6, 2023 22:48
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Nov 6, 2023 •
edited
Loading

@rohanKanojia @rhuss unless you tell me we should include the healthcheck support in this PR, I think this is ready to be reviewed (and merged eventually).

Please note that the 3 Sonarcloud bugs are not on me - they have been present before. Let me know if you want me to include a fix for them.

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 9, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug D 3 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

88.7% 88.7% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.21) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@rhuss
Copy link
Collaborator

rhuss commented Nov 10, 2023

Cool, thanks! I'm just on the road and will be back next week to have a look. But maybe @rohanKanojia is faster :)

Thanks again ...

@rohanKanojia
Copy link
Member

unless you tell me we should include the healthcheck support in this PR, I think this is ready to be reviewed (and merged eventually).

@poikilotherm : Thanks a lot for your work! It looks good to me.

In my opinion, healthcheck support could be added in a follow up PR. Let me merge this one.

@rohanKanojia rohanKanojia merged commit 88fb191 into fabric8io:master Nov 11, 2023
8 of 9 checks passed
@poikilotherm poikilotherm deleted the 888-long-depends-on branch December 1, 2023 16:10
@poikilotherm poikilotherm mentioned this pull request Dec 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohanKanojia rohanKanojia rohanKanojia approved these changes

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 2.1 depends_on conditions not supported
3 participants
@poikilotherm @rohanKanojia @rhuss