OGM-482 EAP JBoss Module #307
Conversation
|
Hi Davide, I think we need two submodules rather than playing with environment variables: when releasing, I need you to release both modules not one. I like the idea of a |
|
About slot names; I saw you're using I really think we should include the version number in the slot. It could be `slot="ogm-$version" ? |
|
Why is it that you decided to have separate modules for each backend (nice) but Infinispan is fully included in the OGM module? |
|
Can we do something about that? and are you sure it needs to be exported too? |
|
Could you explain about this one? And why is it needed by some dialects but not all of them? |
|
I see Neo4J is including the Lucene jar, you could use the module in Widlfly: |
|
This seems required by several modules. Are you sure ? |
|
I suspect we're a bit lacking in integration tests :-/ |
|
P.S. WildFly 8.0.1.Final might be released this week, if it wasn't tagged already (yes it's very late), so if you see some obvious improvement to change in the current structure of WildFly's provided modules, that could still make it but you'll need to focus on it. |
|
Are you sure about needing the older ANTLR and StringTemplace libraries in the HQL module? That's unexpected, if they are really required please open a JIRA to have me fix that. |
|
I have to check if it is possible to apply some of your remarks, in the meanwhile:
I've created two submodules under the module project.
This was a requirement when we have created the first OGM module, It might not be needed anymore, I will check. The
I don't remember why we have decided to do this, actually I've thought is something you proposed :)
These versions are taken from the dependencies included in the project. I'm expecting this one is taken from the dependencies of the parser.weare using, unless something else overrides it.
Tests are always lacking. We don't test all the functionality in the containers but we would need a way to run all the "unit" test cases we already have on the container to increase the coverage. I would treat this as a separate issue though.
ok
Ok, I guess that when we have created the module the version was different and I didn't change the modules when I've upgraded the test to use WildFly 8 |
Note that I changed the module name: would that be a problem interfering with org.hibernate modules?
That's probably how things where before we actually had more modules.
+1 For a follow up which expands on testing, but -1 for no tests at all of these modules: if you don't even have a basic operations tests I'm sure they won't work. If you think you need to simplify, maybe we should cut on the provided modules? |
|
Apart the Neo4j one, which other module is not tested?
|
|
Reading this super fast so that's just a quick comment. I don't want to impose 30s of additional test time nor impose the installation of EAP. |
|
@emmanuelbernard I don't see how we can make modules and not test them. We might want to reorganize tests in proper unit tests, and defer integration testing to CI only? Also note that I expect the work on Docker to speedup overall execution of tests, but I can't serialize this task on that one (and it's just a theory) |
|
@Sanne Search is downloading EAP 6.1 Alpha for testing the modules but the module I've created are tested with EAP 6.2. Which one should I use? |
|
EAP 6.1 Alpha is the only one available in Maven so that's what we need to use :-/ |
As far as I know it is, I've tried to change the group but it does not seem to work. |
There is no need to include it in every module but it is required in the core module with export = "true" |
|
I've rebased the pull request and I've applied the remarks where possible. There are two profiles added, one downloads EAP 6.1 from the repository and the other one will use the one passed through a variable. These profiles are activated if specified in the build or if the variable -Deap.home is defined when running the build. |
|
Jenkins, retest this please. |
| --> | ||
| <id>eap</id> | ||
| <properties> | ||
| <version.arquillian>1.0.3.Final</version.arquillian> |
There was a problem hiding this comment.
Do we really need specific Arquillian versions for the two profiles?
There was a problem hiding this comment.
@DavideD, could one version of Arquillian be used?
There was a problem hiding this comment.
It seems we can use the latest version in both scenarios. I will change it
|
@DavideD I just ran the build (in the default config), and this executed the integration tests on WF. What would I need to do run them on EAP? Specify that other profile by name? Is it also possible to run them on WF and EAP in one go? |
|
The default container is wildfly, you can run the test using mvn install -Deap or mvn install -Deap.home=<jboss_home> Arquillian does not allow two different arquillian containers on the same classpath, the test of both containers would require the creation of two additional modules for each datastore (or something like that). Anyway there is a discussion about this on the mailing list: http://web.archiveorange.com/archive/v/SMVGgmRVpCrtD6zS7e4T |
|
Sorry, I made a mistake, you can use this two profiles:
|
Got it. I originally thought it would be good to be able to run both configurations as part of one Maven build (which could be done via different Maven modules as you say), but probably that's not even required and we can do this solely on the CI server via a Matrix job. Thinking along these lines, we could have just a single integration test module which is configurable on two axes:
By default (i.e. when running |
| </profile> | ||
| <profile> | ||
| <!-- | ||
| This profile will download EAP 6.1.Alpha1 from the Red Hat repository but it requires |
There was a problem hiding this comment.
Can we add the URL of that repo to this comment? I don't know where that one lives.
There was a problem hiding this comment.
It's in the settings-example.xml, not sure if I want to duplicate it here in the commments
There was a problem hiding this comment.
Ah, just saw it in the settings as well. That's fine then.
|
Hi, Persistence unit: Exception: Entity code: @Entity
public class Report<T> implements Serializable {
@Any(metaColumn = @Column(name = "target_type"))
@AnyMetaDef(idType = "long", metaType = "string",
metaValues = {
@MetaValue(targetEntity = Project.class, value = "P"),
@MetaValue(targetEntity = Release.class, value = "R"),
@MetaValue(targetEntity = Artifact.class, value = "A")
})
@JoinColumn(name = "target_id")
private T target;
...It probably doesn't really relate to this PR, but I hope you could help me. Thanks, Honza |
…EAP and for Wildfly
* Update copyright * CouchDB dependencies should be always included * Add relative path in the pom * Obtain properties from the parent
* stringtemplate is not required * anltr is included by core * only ehcache requires slf4j * org.jboss.as.jpa.hibernate is needed only in core * dom4j is not required in the datastore modules * org.jboss.jts is only requires in the core module
|
@janinko I'm pretty sure the problem you are having is not related to this issue but it is a problem of the way you are mapping the entity. My guess is that you are using generics as type for target, you could try with a common interface or Object |
|
I've rebased everything to the latest changes and unified the arquillian version. |
|
And do I understand it correctly that when I add the neo4jPU persistence unit, EAP uses |
|
@janinko The latest OGM requires hibernate core 4.3.1 so if you include the module in your application it will include that version in the classpath. |
| <configuration> | ||
| <artifactItems> | ||
| <artifactItem> | ||
| <groupId>${project.groupId}</groupId> |
There was a problem hiding this comment.
I feel we shouldn't mess with the modules dir of the external installation but instead assume it has been updated accordingly. This might not be what users with a separat inst. might expect.
There was a problem hiding this comment.
It might be useful to run the integration test an a complete custom installation but when I remove the unpacking of the modules from the pom the build does not work. It seems some dependncies are missing. I would prefer to solve this as a separte issue and include what we have at the moment.
|
@DavideD Nice stuff, this makes the demo work on EAP :) In addition to the inline comments, some more remarks:
|
| <relativePath>../pom.xml</relativePath> | ||
| </parent> | ||
|
|
||
| <artifactId>hibernate-ogm-modules-wildfly</artifactId> |
There was a problem hiding this comment.
I feel we should name it hibernate-ogm-modules-wildfly8 (and hibernate-ogm-modules-eap6 or hibernate-ogm-modules-jbossas7).
|
I've rebased and added a couple of new commits to apply the remarks.
Thanks, good to know :)
Updated
I think our goal at the moment is to make it work with EAP and WildFly. The fact that it works with JBoss AS 7 is because of the way EAP is built. We did not test it with the community version.
Now the name of the modules artifact are:
|
|
@DavideD Any idea what's wrong with the test build? |
|
Jenkins, retest this please |
|
In the console, there is this error during the creation of the distribution package: It happens for the pull requests only, does it ring any bells? |
|
Rebased and applied. Many thanks for this, Davide! I've created OGM-499 for some of the discussed follow-ups. Regarding the build failure I'm not sure; I don't see the issue locally. Let's see whether it occurs on master as well. |
https://hibernate.atlassian.net/browse/OGM-482
This patch will create a module for Wildfly and a module fo EAP 6.2.
Setting the environment variable EAP_HOME will make the integration test run using EAP instead of WildFly. Otherwise wildfly is used. Notice that only one container is used for the integration tests.
Is this an accettable solution? Not sure how to run on both containers during the build without creating other submodules, anyone has a nice solution?
To activate the EAP profile I've opted for the activation through an environment variable because is consistent with all the other projects but I would be prone to use a system variable and run
mvn -Deap.home=$EAP_HOMEinstead. What do you guys think?The module will replace the default javax.persistence.api and the default javax.ws.rs.api modules on EAP because it needs later versions.