-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Reactive Messaging codestart #21068
Reactive Messaging codestart #21068
Conversation
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
@vgallet You will need to add:
|
My bad, I wasn't aware codestart could dynamic depending on the selected connector, I thought created a codestart per connector implementation. Can you show me some documentation or an example to do that ? |
For the dynamic config depending on the connector, I guess you'll need to do some Qute templating. For the code of For example: That would mean configuring 4 connectors inside Also, we can name the class according to the connector |
Please can you take a look at this draft and how I created a dynamic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to add the QuarkusCodestartTest, it will also help you make sure it works fine.
...resources/codestarts/quarkus/extension-codestarts/reactive-messaging-codestart/codestart.yml
Outdated
Show resolved
Hide resolved
...ion-codestarts/reactive-messaging-codestart/base/src/main/resources/application.tpl.qute.yml
Outdated
Show resolved
Hide resolved
...ion-codestarts/reactive-messaging-codestart/base/src/main/resources/application.tpl.qute.yml
Outdated
Show resolved
Hide resolved
...ion-codestarts/reactive-messaging-codestart/base/src/main/resources/application.tpl.qute.yml
Outdated
Show resolved
Hide resolved
...ion-codestarts/reactive-messaging-codestart/base/src/main/resources/application.tpl.qute.yml
Outdated
Show resolved
Hide resolved
...resources/codestarts/quarkus/extension-codestarts/reactive-messaging-codestart/codestart.yml
Outdated
Show resolved
Hide resolved
@ia3andy @ozangunalp Hi, I fixed your comments and added an integration test. Nevertheless, I added three TODO as I'm stuck to make it work and make it right :
Thanks for you help |
I will check it out to give it a try and see what's wrong. |
Ok, I've checked with @mkouba, we can't have a counter in the loop. I think we should not comment (until we find a proper way), the multi-connectors case is an edge case. I will have another look tomorrow morning with a bit more time, let's wait until then.. |
Hey @vgallet, I prepared a commit with the fixes to make it work: I had to improve a bit the test framework to avoid conflicts between the different tests.. Let me know if you want me to push it to your branch directly.. |
I had to re-commit due to a problem: ia3andy@d9b66a8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. ✖ This workflow run has failed but no jobs reported an error. Something weird happened, please check the workflow run page carefully: it might be an issue with the workflow configuration itself. |
@ia3andy would you know how to test codestarts in local? |
@ozangunalp, you can either use the tests, or the CLI: |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building d83c72f
Failures⚙️ Initial JDK 11 Build #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖ |
@vgallet could you run |
* fix quarkusio#20872 * created `MyReactiveMessagingApplication` to demonstrates `@Emitter`, `@Incoming` and `@Outgoing` annotations. * created dynamic application.yml based on selected extension * added codestart integration test
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building c64c335
Full information is available in the Build summary check run. Failures⚙️ Devtools Tests - JDK 11 #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖
✖
✖
✖
✖
✖
⚙️ Devtools Tests - JDK 11 Windows #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖
✖
✖
✖
✖
✖
✖
✖
📦 integration-tests/devtools/target/quarkus-codestart-test/1637140781243/real-data/java✖
|
Hello guys @ia3andy @ozangunalp! I discussed with @loicmathieu about the failing tests and he suggested me to split the test BTW, it misses the annotation |
I will try to run the tests locally to see what's going on. I don't think we need to split.
You don't need it, it your case you want that test to be run. |
I pushed fixes, let see what the CI has to say to it :) If it's green, could you squash it all? Thanks @vgallet ✌️ |
Ok the tests are failing because there is no docker environment on CI for the devservices to be started :-/ I am not sure what's the way forward.. |
It's weird that the same test is only failing on windows:
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 4bae693
Full information is available in the Build summary check run. Failures⚙️ Devtools Tests - JDK 11 Windows #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖
✖
📦 integration-tests/devtools/target/quarkus-codestart-test/quarkus-smallrye-reactive-messaging-kafka-cb1a4d5c-075b-450f-a3c5-1eb4ea471a6a/real-data/java✖
|
@cescoffier do you know why this test is not working on Windows? |
Nope, it should not spend on the OS |
@ia3andy Windows CI didn't have Docker, all IT that needs Docker are not run inside the Windows job. |
Should we disable this build test on Windows then? |
As far as I know, all test that requires Docker are disabled in the CI. |
Ok what do we suggest users in that matter? Maybe we should disable this test directly in the template so that users know it doesn't work on Windows? The problem is not related to the codestart test but more the generated project IMO. |
@cescoffier what is our best practice for users regarding dev services using docker and Windows? |
@ia3andy Devservices works on Windows, I use them everyday. But our Windows CI runner didn't have Docker so you must disabled the test via |
Thanks, I'll add this then :) |
Failing Jobs - Building eca87b6
Full information is available in the Build summary check run. Failures⚙️ Devtools Tests - JDK 11 #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖
✖
✖
✖
✖
✖
|
@famod any idea why we don't run devtools tests also on JDK 17 |
I've found the problem and fixed it on my branch, @vgallet could you rebase from main so I can apply it? |
Well, there simply wasn't a Java 16 job when I updated to 17. See also: #21481 (comment) CI has so many jobs already, but as devtools ITs "only" take ~30m I think this could be added. cc @gsmet |
MyReactiveMessagingApplication
to demonstrates@Emitter
,@Incoming
and@Outgoing
annotations.