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

Add MessageConsumer XA Commit and Rollback tests for artemis-jms #320

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

DreamPearl
Copy link

Resolves #319

@zhfeng
Copy link
Contributor

zhfeng commented Nov 2, 2023

Thanks @DreamPearl for your contributions!

Copy link
Contributor

@turing85 turing85 left a comment

Choose a reason for hiding this comment

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

A thanks from me too @DreamPearl

@zhfeng
Copy link
Contributor

zhfeng commented Nov 2, 2023

@turing85 The first check is failing, is there anything we need to concern?

@turing85
Copy link
Contributor

turing85 commented Nov 2, 2023

@turing85 The first check is failing, is there anything we need to concern?

You mean the recreate-comment? No. This is a security feature / quirk of github. PRs of forked projects do not get the permission to create comments. We'd need to restructure the pipeline to "make this work". But since PRs from forks are few and far between, it isn't worth the effort.

@zhfeng
Copy link
Contributor

zhfeng commented Nov 2, 2023

Hmm, I get it. Anyway, it seems better if we can make it work against the PRs from forked projects.

@zhfeng
Copy link
Contributor

zhfeng commented Nov 2, 2023

@turing85 Do you think we need backport these tests to 3.0.x at least?

Copy link
Contributor

@turing85 turing85 left a comment

Choose a reason for hiding this comment

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

Sorry, found something. This is only indirectly related to the PR. The exception case was not triggered until now, and I suspect you copied from the ArtemisJmsConsumerManager. Could you incorporate these changes?

@turing85
Copy link
Contributor

turing85 commented Nov 2, 2023

@turing85 Do you think we need backport these tests to 3.0.x at least?

I already marked #319 as backport/3.0.x and backport/2.x 🙂

@turing85
Copy link
Contributor

turing85 commented Nov 2, 2023

@DreamPearl one last request: could you squash the PR down to one (semantic) commit? This will simplify cherry-picking for the backports 🙂

@DreamPearl
Copy link
Author

@DreamPearl one last request: could you squash the PR down to one (semantic) commit? This will simplify cherry-picking for the backports 🙂

Done!

Copy link
Contributor

@middagj middagj left a comment

Choose a reason for hiding this comment

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

Left a small comment, but let's not overcomplicate adding a test. Thanks!

Add MessageConsumer XA Commit and Rollback tests

Add XA Commit and Rollback tests
@turing85 turing85 merged commit 7fb8105 into quarkiverse:main Nov 3, 2023
37 of 38 checks passed
@turing85
Copy link
Contributor

turing85 commented Nov 3, 2023

@all-contributors Please add @DreamPearl for code

Copy link
Contributor

@turing85

I've put up a pull request to add @DreamPearl! 🎉

@DreamPearl
Copy link
Author

@turing85 @zhfeng @middagj Thanks! :)

We need these xa tests. May I ask when the next release will be?
Can we have one with the latest xa-tests changes as early as possible? I will help if needed.
Btw what’s the right platform to discuss this? Thanks in advance!

@turing85
Copy link
Contributor

turing85 commented Nov 3, 2023

@turing85 @zhfeng @middagj Thanks! :)

We need these xa tests. May I ask when the next release will be? Can we have one with the latest xa-tests changes as early as possible? I will help if needed. Btw what’s the right platform to discuss this? Thanks in advance!

What do you mean by "we need these tests"? Do you need them on your end? We normally do not release for minor changes, but I am open to produce a release.

@louisa-fr @middagj @zhfeng What do you think? Can we produce a release? If we do, I would suggest producing a patch-level release on all three branches (main, 3.0.x and 2.x)

@DreamPearl
Copy link
Author

What do you mean by "we need these tests"? Do you need them on your end? We normally do not release for minor changes, but I am open to produce a release.

Ah cool. In that case, we will just backport these tests to our branch. Sorry to bother and thanks!

@turing85
Copy link
Contributor

turing85 commented Nov 3, 2023

What do you mean by "we need these tests"? Do you need them on your end? We normally do not release for minor changes, but I am open to produce a release.

Ah cool. In that case, we will just backport these tests to our branch. Sorry to bother and thanks!

Let's see what the team says 🙂 as I said: I am open to producing a release.

@middagj
Copy link
Contributor

middagj commented Nov 3, 2023

We don't release tests, do we? If someone can be helped with a release of some small fixes, I am okay with it. But I don't see what we release here.

@turing85
Copy link
Contributor

turing85 commented Nov 3, 2023

We don't release tests, do we? If someone can be helped with a release of some small fixes, I am okay with it. But I don't see what we release here.

Huh... true. I double-checked. The jars that we release only contain the non-test code.

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

Successfully merging this pull request may close these issues.

Add artemis-jms XA tests
5 participants