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

OGM-1287 Create a property to look up native nosql connection via JNDI #1045

Closed
wants to merge 10 commits into from

Conversation

fax4ever
Copy link
Contributor

@fax4ever fax4ever added the Preview This is a preview PR, some tests may not work. Please do not merge it. label Jun 18, 2018
@fax4ever fax4ever force-pushed the 1287-native-client-lookup branch 2 times, most recently from 128400a to bbcbc71 Compare June 20, 2018 10:26
@fax4ever fax4ever added Ready for review and removed Preview This is a preview PR, some tests may not work. Please do not merge it. labels Jun 20, 2018
@fax4ever fax4ever requested a review from DavideD June 20, 2018 10:27
[NOTE]
====
If you have provisioned your WildFly with Hibernate Ogm featurepack(s),
Copy link
Member

Choose a reason for hiding this comment

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

s/Ogm/OGM/

----

At this point you can use Hibernate property `hibernate.connection.resource` in your _persistence.xml_,
Copy link
Member

Choose a reason for hiding this comment

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

s/can use Hibernate property/can use the Hibernate property/

@fax4ever
Copy link
Contributor Author

Thank you @Sanne for the typo fixes. I applied them and rebased the branch.

Copy link
Member

@DavideD DavideD left a comment

Choose a reason for hiding this comment

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

Looks good overall, would it be possible to add some tests in the integrationtest module?

}

// clear resources
this.jndiService = null;
Copy link
Member

Choose a reason for hiding this comment

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

Not critical but I find it counter intuitive that the lookup method also does the clear, can we move it at the end of the start method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can

}

// clear resources
this.jndiService = null;
Copy link
Member

Choose a reason for hiding this comment

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

Not critical but I find it counter intuitive that the lookup method also does the clear, can we move it at the end of the start method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can

@DavideD
Copy link
Member

DavideD commented Jul 3, 2018

would it be possible to add some tests in the integrationtest module?

Are you working on this?

@fax4ever
Copy link
Contributor Author

fax4ever commented Jul 3, 2018

@DavideD good idea. I'm going to start to work on it.

@fax4ever fax4ever added Preview This is a preview PR, some tests may not work. Please do not merge it. and removed Changes suggested labels Jul 4, 2018
@fax4ever fax4ever removed the Preview This is a preview PR, some tests may not work. Please do not merge it. label Jul 4, 2018
@DavideD
Copy link
Member

DavideD commented Jul 5, 2018

Merged #1045

Thanks @fax4ever!
I had to change a commit message because the issue was wrong and fix a minor thing in the documentation

@DavideD DavideD closed this Jul 5, 2018
@fax4ever
Copy link
Contributor Author

fax4ever commented Jul 5, 2018

Thank you @DavideD

@fax4ever fax4ever deleted the 1287-native-client-lookup branch July 5, 2018 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants