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

Integrate JPA into Shamrock #22

Merged
merged 2 commits into from
Sep 17, 2018
Merged

Integrate JPA into Shamrock #22

merged 2 commits into from
Sep 17, 2018

Conversation

emmanuelbernard
Copy link
Member

Hey @stuartwdouglas and all,

Can you review this PR and also test it locally to let me know if it all works fine for you.

It expects a docker engine to run locally or via Docker for macos.

A couple of points to review specifically:

  • jpa-strict is a trimmed down version of strict to help track problems more easily. Sanne felt it easier to keep it around. If has quite a few dups compared to strict but I guess that's ok. If you see areas were we could strip it down further, let me know. I did a lousy initial job.
  • strict also contains the JPA tests
  • I am no expert in Maven, let me know if my docker-maven-plugin integration are in the correct phase
    • I did put the plugin version in each project (did not know how to share it)
    • I start the postgres container during the compile phase so that it is started for the unit test runs. Note that I stop it fist in case the build failed and the container is not properly stopped during the previous run
    • I stop the container during the post-integration-test phase so that it runs during native-image tests and then stops

Mappings to phases might be better. I noticed that your shamrock plugin does not map to phases but I'm not sure why it works for you and not for me in such phaseless setting.

@Sanne
Copy link
Member

Sanne commented Sep 17, 2018

It works for me but only on module shamrock-strict-example: fails on shamrock-strict-jpa-example, in which case it timeouts to connect to the database. Weird as the DB is up so there must be some different underlying reason but I couldn't find one.

P.S. the docker image seems to still be running after a build, so subsequent builds fail because of the TCP port being taken.
I suspect issuing a generic "stop" isn't good enough, but I'm not familiar with this plugin. @kenfinnigan maybe ?

@Sanne
Copy link
Member

Sanne commented Sep 17, 2018

P.P.S. I suspect the "docker stop" you begin with is pointless as the name is randomly assigned during the build, so any attempt to stop it would unlikely match the existing container instances.

@emmanuelbernard
Copy link
Member Author

@Sanne go on PTO :)
It is not pointless, I added it because of failures (port conflict upon second start). Fabric8 is under-documented but I think the alias works as a docker --name and thus makes it unique.

@emmanuelbernard
Copy link
Member Author

It works for me but only on module shamrock-strict-example: fails on shamrock-strict-jpa-example, in which case it timeouts to connect to the database. Weird as the DB is up so there must be some different underlying reason but I couldn't find one.

P.S. the docker image seems to still be running after a build, so subsequent builds fail because of the TCP port being taken.
I suspect issuing a generic "stop" isn't good enough, but I'm not familiar with this plugin. @kenfinnigan maybe ?

Damn, it does work fine here with the container stopping (at least not showing up on docker ps). What command do you run precisely?

@emmanuelbernard
Copy link
Member Author

@Sanne I confirm I see the container go down and a new one showing up between strict and jpa-strict running mvn install from examples (mvn clean install works too).
Maybe dust your docker from any residual postgresql running?

@stuartwdouglas
Copy link
Member

It works for me on both Linux(centos) and Mac, and I think it looks good, so I am going to merge this.

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

Successfully merging this pull request may close these issues.

4 participants