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

Fix Gateway ports syntax for compose #14962

Merged
merged 5 commits into from
May 13, 2021

Conversation

swarajsaaj
Copy link
Contributor

@swarajsaaj swarajsaaj commented May 12, 2021

Fix port string generation in docker-compose.yml file, supporting
app.yml that may contain hostname as well in port-mapping

Fix #14910


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

Fix port string generation in docker-compose.yml file, supporting
app.yml that may contain hostname as well in port-mapping

Fix jhipster#14910
@mraible
Copy link
Contributor

mraible commented May 12, 2021

Thanks for your contribution, @swarajsaaj! I believe the version number needs to be removed too. That's what @ndeloof did when he fixed my project in oktadev/java-microservices-examples#23.

If you want to really test things, we should change all instances of docker-compose to docker compose in test-integration/scripts.

The version in docker-compose.yml file is optional after v1.27

Fix jhipster#14910
@swarajsaaj
Copy link
Contributor Author

swarajsaaj commented May 12, 2021

Thanks for your contribution, @swarajsaaj! I believe the version number needs to be removed too. That's what @ndeloof did when he fixed my project in oktadev/java-microservices-examples#23.

If you want to really test things, we should change all instances of docker-compose to docker compose in test-integration/scripts.

Thanks @mraible ,

  1. Though the version number is optional now, but we can remove it to be more compliant to latest spec.
  2. Yes, it would be great to really test this using integration tests. Made changes in 1460d74

@swarajsaaj
Copy link
Contributor Author

swarajsaaj commented May 12, 2021

The pipelines are failing on adding "docker compose" command in test-integration/scripts. The command seems working fine on my machine. Is it possible the new "Compose CLI" support is not installed on azure devops instance?

Update: Added a new step in azure pipeline to install 'compose-cli' and its successful now

@ndeloof
Copy link

ndeloof commented May 13, 2021

Compose CLI is indeed a separate component, that is still beta and won't be installed by default.
Probably simpler to use the classic docker-compose until new compose is distributed within the docker CLI system packages

@swarajsaaj
Copy link
Contributor Author

Yes @ndeloof , its still in "Tech Preview" and the pipelines worked only after adding a step to install the component first.
@mraible Shall we defer it from our test scripts and add it later once this feature is stable (and bundled by default) ?

@mraible
Copy link
Contributor

mraible commented May 13, 2021

@swarajsaaj Probably. I was motivated to add support for it since docker-compose tells you to try it.

Reverting to docker-compose for integration tests, as compose-cli
(docker compose) is in preview mode and not installed by default.

Fix jhipster#14910
@swarajsaaj
Copy link
Contributor Author

@mraible Okay then let's keep the stable 'docker-compose' support for now, and migrate tests to new command once it is stable 😄 (BTW the tests showed that docker compose too is compatible with generated compose files now).
Reverted test command change in f6c9238

@mraible mraible merged commit b9823de into jhipster:main May 13, 2021
@swarajsaaj
Copy link
Contributor Author

@pascalgrimaud
Copy link
Member

@swarajsaaj : approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker Compose sub-generator generates YAML that doesn't work with docker compose
6 participants