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

JBTM-3472 Produce jakarta artifact for narayana-jta #1824

Merged
merged 1 commit into from Apr 29, 2021

Conversation

beikov
Copy link
Contributor

@beikov beikov commented Apr 22, 2021

https://issues.redhat.com/browse/JBTM-3472

NO_WIN MAIN AS_TESTS QA_JTA QA_JTS_OPENJDKORB TOMCAT XTS
!QA_JTS_JDKORB !QA_JTS_JACORB !BLACKTIE !PERF !RTS !JACOCO !LRA

@jbosstm-bot
Copy link

⚠️ narayana CI not started.

Author is not the 'narayana' contributor, to permit PR being run members of jbosstm can write comment of text: TESTIT

@mmusgrov
Copy link
Contributor

TESTIT

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@beikov
Copy link
Contributor Author

beikov commented Apr 23, 2021

I think jbosstm/jboss-transaction-spi#29 first needs to be merged and released so that we can refer to that artifact in this PR.

@ochaloup
Copy link
Contributor

ochaloup commented Apr 23, 2021

@beikov our CI script considers the SPI changes for tests but the PR description has to be in the right format (not documented, sorry for that) It requires the full http address to the PR) I changed that and the later test executions have passed (the CORE and XTS).

The run of the AS_TESTS axis is not fully stable in our CI (we experience time to time intermittent failures). The CI run shows failures at StatelessTimeoutTestCase and DatabaseTimerServiceMultiNodeTestCase which are known not related failures which we are working on address.
Then I can see three more which we need to investigate: DupliciteApplicationPathTestCase, JdrReportManagmentTestCase,ConsoleAccessLogTestCase, if they're just intermittent or somehow related to this PR.

@mmusgrov
Copy link
Contributor

FYI I promoted the release of 7.6.1.Final (the tag is https://github.com/jbosstm/jboss-transaction-spi/releases/tag/7.6.1.Final) and hopefully it will eventually be available from maven central (which can take hours).

@mmusgrov mmusgrov changed the title Produce jakarta artifact for narayana-jta JBTM-3472 Produce jakarta artifact for narayana-jta Apr 26, 2021
Copy link
Contributor

@ochaloup ochaloup left a comment

Choose a reason for hiding this comment

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

@beikov I think the fix is nice. Just before merging we need to update the SPI version to 7.6.1.Final at pom.xml (https://github.com/jbosstm/narayana/blob/5.11.1.Final/pom.xml#L483) as discussed already.
Plus, I think the new artifact should be part of the narayana-full.zip. That's a release zip artifact provided as a bundle for anybody looking for a all narayana artifacts. You need to update the assembly/bin.xml at https://github.com/jbosstm/narayana/blob/master/narayana-full/src/main/assembly/bin.xml#L107. I assume in the similar fashion as it's done for narayana-jta.

@mmusgrov I have few doubts on how Narayana project expects to work with this new mvn artifact

  • this change does not consider any testing from Narayana perspective (e.g. when some Jakarta API is changed, I I understand the changeset correctly). Is that ok or should that be addressed somehow? (a note: the current run of the CI it's only important to check if compiles, no other axis is needed to be run as there is no relation to jakarta)
  • for WildFly we use the narayana-jts-idlj.jar dependency. Should there be created some naryana-jts-idlj-jakarta.jar and provided to WildFly?

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

Copy link
Contributor

@ochaloup ochaloup left a comment

Choose a reason for hiding this comment

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

I would like ask for one more adjustment. Otherwise all seems fine.

<properties>
<version.jakarta.transaction.jakarta.transaction-api>2.0.0</version.jakarta.transaction.jakarta.transaction-api>
<version.jakarta.resource.jakarta.resource-api>2.0.0</version.jakarta.resource.jakarta.resource-api>
<version.org.jboss.jboss-transaction-spi>7.6.1.Final</version.org.jboss.jboss-transaction-spi>
Copy link
Contributor

Choose a reason for hiding this comment

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

To me the PR is good for merge. Just I would like to ask for one more adjustment, please mov the version configuration to root pom.xml. I mean redefine the version 7.6.0 here: https://github.com/jbosstm/narayana/blob/master/pom.xml#L485
and remove the line from this pom.xml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

Copy link
Contributor

@ochaloup ochaloup left a comment

Choose a reason for hiding this comment

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

Thank you. Seems good to me. Let's wait for the CI jobs.

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@beikov
Copy link
Contributor Author

beikov commented Apr 29, 2021

The SPI artifacts are on maven central now. Is there anything else you would like me to do before you merge this PR?

Copy link
Contributor

@mmusgrov mmusgrov 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 and ready to merge.

@mmusgrov mmusgrov merged commit 3bebcb2 into jbosstm:master Apr 29, 2021
@beikov beikov deleted the jakarta branch April 29, 2021 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants