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

Make sure we have Agroal datasources before configuring health checks #43991

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Oct 21, 2024

Fixes #43990

@gsmet
Copy link
Member Author

gsmet commented Oct 21, 2024

@michalvavrik I tried to add a test to the quarkus-agroal-deployment module but I couldn't reproduce the issue. I'm not sure what you're doing to trigger it specifically. Do you have an idea of how we could reproduce it in the main repo?

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

LGTM, though I find it odd that the bean would not be resolvable... I suspect the bean having the default scope (dependent), toghether with the use of bytecode recorders, might have something to do with that, so I sent a suggestion to your fork: gsmet#193

@michalvavrik
Copy link
Member

@michalvavrik I tried to add a test to the quarkus-agroal-deployment module but I couldn't reproduce the issue. I'm not sure what you're doing to trigger it specifically. Do you have an idea of how we could reproduce it in the main repo?

I tried, but inside Argoal deployment module you have a JDBC extension (H2) so I think you won't reproduce it there. If you drop it you can reproduce it like this 84813f9. So unless you have different module I don't think you can reproduce it.

@gsmet
Copy link
Member Author

gsmet commented Oct 21, 2024

OK, I think we will have to live with it then. @michalvavrik if you can independently verify the last version of this PR fixes it for you, that would be awesome.

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

I tried the latest version and it fixes my issue. Thank you

@yrodiere yrodiere added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 21, 2024
Copy link

quarkus-bot bot commented Oct 21, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit c9d008b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@yrodiere yrodiere merged commit 097092a into quarkusio:main Oct 21, 2024
43 checks passed
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Oct 21, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Healthcheck results in NPE when no JDBC datasources are defined after breaking changes in 3.17
3 participants