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-650 Contrib repository #850

Closed
wants to merge 12 commits into from

Conversation

@DavideD
Copy link
Member

commented Mar 16, 2017

https://hibernate.atlassian.net/browse/OGM-650

I've removed the part related to Ehcache, Cassandra, CouchDB and Redis

At the moment they are in a new project on my repository: https://github.com/DavideD/hibernate-ogm-contrib

I'll move them on the hibernate repository after the review.

@DavideD DavideD self-assigned this Mar 16, 2017
@DavideD DavideD requested a review from gsmet Mar 16, 2017
@DavideD DavideD force-pushed the DavideD:OGM-650-contrib-repository branch from 245a1aa to 6c32df8 Mar 17, 2017
Copy link
Member

left a comment

I added a few comments and questions but it looks good overall.

It misses some doc about the contrib modules in the main doc and in the README.md but I suppose you were planning on adding that once the dust is settled and the contrib repository moved under the hibernate organization.

I have 3 major questions/remarks about the contrib modules:

  • AFAICS you lost all the history of these modules. I'm wondering if you couldn't keep it but applying the changes on top of a clone of the original repository. I know the history is often very useful and it might be even more if we expect new people to step in to maintain this repository. I don't know how many changes you applied on top of the existing dialects and if they are easily appliable on top of the existing repo. I wouldn't do it in the details (I mean no history for the modules, the integrationtests, the doc... is OK but I think it's important for the various dialects)
  • about the test strategy, I dug a bit further and it looks like you are running the TCK tests and add the excludes in a skipTests file. Am I right? It looks interesting. I'm a bit worried about just using the simple name and not the FQCN though.
  • as for Travis, I think we should try to make it a Matrix build and not use the parent to build all the modules. This way, I think Travis makes the info easier to find when the build fails and it will build all modules. This means that a person working on a specific module would have the results even if another module does not build.
bom/pom.xml Outdated
<artifactId>lettuce</artifactId>
<version>${lettuceVersion}</version>
</dependency>

<!-- RESTEasy for accessing CouchDB -->
<dependency>
<groupId>org.jboss.resteasy</groupId>

This comment has been minimized.

Copy link
@gsmet

gsmet Mar 20, 2017

Member

If we use it only for CouchDB, we should remove it. If not, we should change the comment.

This comment has been minimized.

Copy link
@DavideD

DavideD Mar 20, 2017

Author Member

It's used for Neo4j as well

This comment has been minimized.

Copy link
@DavideD

DavideD Mar 20, 2017

Author Member

I missed the comment, I'll update it

</includes>
</dependencySet>
</dependencySets>
</dependencySets>

This comment has been minimized.

Copy link
@gsmet

gsmet Mar 20, 2017

Member

Looks like there is a space missing here.

This comment has been minimized.

Copy link
@DavideD

DavideD Mar 20, 2017

Author Member

ok

@@ -334,5 +225,5 @@
<include>org.ow2.asm:asm-util</include>
</includes>
</dependencySet>
</dependencySets>
</dependencySets>

This comment has been minimized.

Copy link
@gsmet

gsmet Mar 20, 2017

Member

Same here, looks like there is a space missing now.

This comment has been minimized.

Copy link
@DavideD

DavideD Mar 20, 2017

Author Member

ok

@@ -21,7 +21,7 @@
* @author Emmanuel Bernard &lt;emmanuel@hibernate.org&gt;
*/
@SkipByGridDialect(
value = { GridDialectType.CASSANDRA, GridDialectType.INFINISPAN_REMOTE },
value = { GridDialectType.INFINISPAN_REMOTE },

This comment has been minimized.

Copy link
@gsmet

gsmet Mar 20, 2017

Member

This bugs me a little. We are losing information and I would have expected the TCK to be used for the contrib dialects. It looks like you're reintroducing the ability to do that in a further commit. What's your strategy exactly for that?

We agree that you have moved this information in the skipTests file of each contrib module?

This comment has been minimized.

Copy link
@DavideD

DavideD Mar 20, 2017

Author Member

Yes, I moved the tests in the skipTests files

@DavideD

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2017

AFAICS you lost all the history of these modules. I'm wondering if you couldn't keep it but applying the changes on top of a clone of the original repository. I know the history is often very useful and it might be even more if we expect new people to step in to maintain this repository. I don't know how many changes you applied on top of the existing dialects and if they are easily appliable on top of the existing repo. I wouldn't do it in the details (I mean no history for the modules, the integrationtests, the doc... is OK but I think it's important for the various dialects)

OK, It makes sense. I see what I can do.

about the test strategy, I dug a bit further and it looks like you are running the TCK tests and add the excludes in a skipTests file. Am I right? It looks interesting. I'm a bit worried about just using the simple name and not the FQCN though.

I'm open to other solutions but I think I prefer to have a way for each dialect to skip it's own tests.
I guess I can use the whole name. It's probably safier

as for Travis, I think we should try to make it a Matrix build and not use the parent to build all the modules. This way, I think Travis makes the info easier to find when the build fails and it will build all modules. This means that a person working on a specific module would have the results even if another module does not build.

it should fail only if the person changed something affecting the other modules. It's a good idea anyway.

@DavideD

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2017

I'll let you know when I applied the changes

@gsmet

This comment has been minimized.

Copy link
Member

commented Mar 20, 2017

it should fail only if the person changed something affecting the other modules. It's a good idea anyway.

Yeah, my guess is that when we will make an infrastructure change to the core, the contrib dialects won't be adapted right away so some may fail for some time. Having Travis for the one you work on even in this case looks like a good idea to me.

DavideD added 11 commits Mar 14, 2017
  We are removing from the documentation, integrationt tests and wildfly modules
  the following:

  * Cassandra
  * Redis
  * CouchDB
  * EhCache

  We are moving them in a separate repository for contributed dialects.
  Tests for Cassandra, CouchDB, Redis and Ehcache won't be run anymore.

  It's now responsability of the contrib repository to execute this tests.
  Contributors can use it to run integration tests on dialects
  that are not in the main repository
  This way dialects that are not in the main repositoy can re-use the annotation
  `@SkipByGridDialect`.

  Contributed dialects can ignore tests listing them in a text file.
@DavideD DavideD force-pushed the DavideD:OGM-650-contrib-repository branch from 6c32df8 to c5476f9 Mar 27, 2017
@DavideD

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2017

@gsmet I've applied the remarks and updated the other repository as well, History included

@gsmet

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

@DavideD

Looks good. Logging things we should not forget about: update the README.md, clean up the .travis.yml file (there are Redis and Cassandra specific stuff in it).

I'm wondering if we should consider changing the groupId in the contrib repo to org.hibernate.ogm.contrib (not sure it's recommended to use a subgroup or if we have to use something like org.hibernate.ogm-contrib). WDYT? It's probably something we should discuss with the others.

I think it's probably worth a general email on the list to check that we're all on the same page before merging this.

Nice work.

@DavideD

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

I've added a commit to move some properties in the BOM so that they are consistent in the new repositories. The repository for CouchDB, Cassandra, EhCache and Redis are currently on my account but I'll create them in the hibernate one if they are OK:

https://github.com/DavideD/hibernate-ogm-redis
https://github.com/DavideD/hibernate-ogm-couchdb
https://github.com/DavideD/hibernate-ogm-ehcache
https://github.com/DavideD/hibernate-ogm-cassandra

They will need some additional work in regard to the documentation and possibly something else but I think they are a good starting point.

@gsmet

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

Merged and added a couple of commits on top of that.

@gsmet gsmet closed this Apr 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.