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

Improve compose files by using DNS alias #5135

Merged
merged 3 commits into from Feb 15, 2017

Conversation

pascalgrimaud
Copy link
Member

Related to #5128

  • remove all container_name, links and external_links to use DNS alias instead
  • add npm test

@PierreBesson : if you have time to review it

@jdubois / @mraible : I know you do a lot of demos, so I hope you can test this. I don't want to break something!

- remove all container_name, links and external_links to use DNS alias instead
- add npm test
Copy link
Contributor

@PierreBesson PierreBesson left a comment

Choose a reason for hiding this comment

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

I have tested a full setup and it worked without problems. But this might need more testing so that we don't break anything.

@PierreBesson
Copy link
Contributor

@pascalgrimaud I have notice a new docker-compose feature that could replace our JHipster Sleep by using docker healthchecks and depends_on : https://docs.docker.com/compose/compose-file/#/dependson

Also big news, I have managed to deploy a docker-compose with a full microservice stack to a swarm, with scaling working correctly. So when I have the time I will finally be able to add the option to generate a docker-compose for "prod".

@romaindequidt
Copy link
Contributor

@pascalgrimaud
Copy link
Member Author

@PierreBesson : interesting! so we need to upgrade the docker-compose file version to 2.1+. I think it can be done on another PR to be clearer

@romaindequidt : sure, it looks better than what I did:

  • you can PR directly on my branch (so this PR will be updated)
  • or you can wait this PR to be merged, then you make a PR, so you get credits for your work

@romaindequidt
Copy link
Contributor

@pascalgrimaud I wanted to PR our PR but i do not if i can (i'm not jhipster member and I did find your branch :/)
Sorry to ask you that but, may you explain me the procedure?

@pascalgrimaud
Copy link
Member Author

@romaindequidt : so if you want to PR on my branch, just go to this link pascalgrimaud/generator-jhipster@docker-1.13...romaindequidt:docker-1.13

@jdubois
Copy link
Member

jdubois commented Feb 15, 2017

LGTM so @pascalgrimaud I let you merge this when the PR on your PR is also merged :-)

@pascalgrimaud pascalgrimaud merged commit ab8bef5 into jhipster:master Feb 15, 2017
@romaindequidt
Copy link
Contributor

romaindequidt commented Feb 15, 2017

@jdubois @pascalgrimaud Are you sure to force everyone to upgrade their Docker client version?
For exemple, some AWS service are only available for 1.12 version:
Amazon ECS Container Agent Versions
Should we add a subgenerator option/parameter to choose one of the versions?
If yes, i can work on it.

@jdubois
Copy link
Member

jdubois commented Feb 15, 2017

Thanks @romaindequidt I didn't notice that!!!
No we can't support multiple versions, that would be a lot of work for something quite temporary. Our options are:

  • Stay on the newer version, and if people want to use it, they need to upgrade (or stay on an old JHipster release otherwise)
  • Switch back to the older version, and in that case people won't benefit from the newer code

I would stay with 1.13, people need to upgrade, or use an up-to-date cloud. @pascalgrimaud what's your opinion?

@pascalgrimaud
Copy link
Member Author

In fact, this PR removes links and external_links, to use DNS alias instead.
It's not concerned by Docker 1.13 -> the title was wrong, it should be: Improve compose files by using DNS alias
The version in Docker compose files is still v2, not v3 (Docker 1.13), so don't worry @romaindequidt it is still compatible :)

But I made this PR jhipster/jhipster.github.io#386 to update the Docker client API to 1.13

@pascalgrimaud pascalgrimaud changed the title Improve compose files with Docker 1.13 Improve compose files by using DNS alias Feb 15, 2017
@romaindequidt
Copy link
Contributor

@pascalgrimaud Ok, if it's not this PR it will be the next one ;)
I 'd like to work on the "healthchecks" and "depends_on" parameters proposed by @PierreBesson , so in that case we go forward and we do not manage "backward compatibility"

@pascalgrimaud
Copy link
Member Author

Yes plz @romaindequidt !

@PierreBesson
Copy link
Contributor

@romaindequidt Yes a PR would be very much appreciated ! Do not worry about "background compatibility" as for us docker-compose is first and foremost a "dev" tool (no meaningful way to manage your "prod" with it before the last version).

@jdubois jdubois modified the milestone: 4.0.4 Feb 17, 2017
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.

None yet

4 participants