Skip to content

Conversation

moulalis
Copy link
Contributor

@moulalis moulalis commented Apr 10, 2023

@mmusgrov
Copy link
Member

Where are the quickstart instructions?
Do we still have the problem of filters not getting registered?
The PR, as it stands, is incomplete and perhaps needs to be withdrawn.

@moulalis
Copy link
Contributor Author

Where are the quickstart instructions? Do we still have the problem of filters not getting registered? The PR, as it stands, is incomplete and perhaps needs to be withdrawn.

Hi @mmusgrov, This is Draft PR, it is not ready for review.

@mmusgrov
Copy link
Member

What is the purpose of this PR if it's not a request to merge a fix into the codebase?

@mmusgrov
Copy link
Member

What is the purpose of this PR if it's not a request to merge a fix into the codebase?
When you raise a PR various engineers get email notifications that they should review the request so if you raise one before it's ready then it just adds workload to the requested engineers.

@moulalis moulalis force-pushed the JBTM-3462 branch 2 times, most recently from 7541237 to f3d9d71 Compare April 11, 2023 18:49
@moulalis moulalis marked this pull request as ready for review April 11, 2023 18:49
@moulalis
Copy link
Contributor Author

PR is ready for review.
Thanks.

@moulalis moulalis force-pushed the JBTM-3462 branch 2 times, most recently from e944565 to 4475c6d Compare April 12, 2023 05:24
@marcosgopen
Copy link
Member

Thank you @moulalis for your PR. I wonder if the sra module should go under rts/at/ (as Moulali proposed) or under rts/ folder like lra module. WDYT @mmusgrov ?

@mmusgrov
Copy link
Member

Thank you @moulalis for your PR. I wonder if the sra module should go under rts/at/ (as Moulali proposed) or under rts/ folder like lra module. WDYT @mmusgrov ?

SRA is synonymous with REST-AT - it's just a different name for it. The SRA demo is to showcase how to use the REST-AT annotations so it belongs with the at module. I called it SRA to complement LRA when I proposed the model to the MP Eclipse organisation - MP projects are annotation based so that's why I needed to add them for REST-AT which currently only offers a pure REST API. If people think it is going to cause confusion we could change the name but TBH the name SRA seems like a good fit for the annotation API layered over the REST API.

Copy link
Member

@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.

We have lost a record of the work that other contributors (such as Sweta and Ondra) made to the module (git log --follow should be able to track the moved files).

@mmusgrov
Copy link
Member

mmusgrov commented Apr 12, 2023

@moulalis I don't think we should merge this until we have fixed the demo ie issue https://issues.redhat.com/browse/JBTM-3489 should be resolved first. If we were to merge this first then JBTM-3489 would need to be marked as a blocker.

I've linked the two issues.

@mmusgrov
Copy link
Member

mmusgrov commented Apr 12, 2023

Resolving JBTM-3463 would also significantly improve this quickstart

@marcosgopen
Copy link
Member

marcosgopen commented Apr 12, 2023

Resolving JBTM-3463 would also significantly improve this quickstart

Yes, I agree with Mike. It would be great to resolve all the issues related to the SRA quickstart (same PR but different commits). I understand it might be quite demanding, so if you need help we will be happy to help.

@mmusgrov
Copy link
Member

Resolving JBTM-3463 would also significantly improve this quickstart

Actually, working on this issue could be a follow up quickstart showing how to decompose the example into separate microservices and also showing how the context still gets propagated.

@moulalis
Copy link
Contributor Author

Resolving JBTM-3463 would also significantly improve this quickstart

Actually, working on this issue could be a follow up quickstart showing how to decompose the example into separate microservices and also showing how the context still gets propagated.

FYI, I have already started working on this. @mmusgrov @marcosgopen

@moulalis moulalis force-pushed the JBTM-3462 branch 2 times, most recently from d51728d to c31d277 Compare April 17, 2023 07:18
@moulalis
Copy link
Contributor Author

Hi @mmusgrov, @marcosgopen
Please mark this PR as HOLD.

@jmfinelli jmfinelli added the Hold label Apr 17, 2023
@mmusgrov
Copy link
Member

@moulalis I'd have expected you to be able to add the label since I gave you the "Write" Role in github - did you try it first.
Also the expectation is that the PR description should be updated to indicate why the PR is on Hold, do you have the permission to update the PR description?

@moulalis
Copy link
Contributor Author

@moulalis I'd have expected you to be able to add the label since I gave you the "Write" Role in github - did you try it first. Also the expectation is that the PR description should be updated to indicate why the PR is on Hold, do you have the permission to update the PR description?

I apparently added the comment. I realized that I have permission until then @jmfinelli added it.

The HOLD label is needed until https://issues.redhat.com/browse/JBTM-3489 is fixed.

@mmusgrov
Copy link
Member

mmusgrov commented Apr 20, 2023

@moulalis thanks.

Also, did you notice my comment about the commit history (#434 (review))

@moulalis
Copy link
Contributor Author

No changes updated, only rebase is done.

rts/at/pom.xml Outdated
<groupId>org.jboss.narayana.quickstart.rts</groupId>
<artifactId>rts-quickstarts</artifactId>
<version>7.0.1.Final-SNAPSHOT</version>
<version>6.0.2.Final-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

Here you need version 7.0.1.Final-SNAPSHOT

@moulalis
Copy link
Contributor Author

moulalis commented Aug 7, 2023

@mmusgrov @marcosgopen I've updated the PR along with CI tests, could you please review and run the job?

@mmusgrov
Copy link
Member

mmusgrov commented Aug 7, 2023

Does it work now?

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

@moulalis
Copy link
Contributor Author

moulalis commented Aug 7, 2023

Does it work now?

Yes

Copy link
Member

@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.

The contribution looks good, thanks @moulalis for your continued perseverance to see this one through to the end. @marcosgopen do you think that we can merge this one now?

Copy link
Member

@marcosgopen marcosgopen left a comment

Choose a reason for hiding this comment

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

Thanks @moulalis for your great work on this PR. It looks good to me.

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

Successfully merging this pull request may close these issues.

10 participants