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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use uppercase UP in ReactiveDatasourceHealthCheck #26418

Merged
merged 1 commit into from Jun 29, 2022
Merged

Use uppercase UP in ReactiveDatasourceHealthCheck #26418

merged 1 commit into from Jun 29, 2022

Conversation

luckyguy73
Copy link
Contributor

@luckyguy73 luckyguy73 commented Jun 28, 2022

request to change the data default value to uppercase to match the status case. the status enum in HealthCheckResponse is uppercase, so this would make them both the same case for consistency, which is good for those of us with OCD and looks good when getting values from json from q/health or viewing webpage at q/health-ui 馃榾

image

the status enum in HealthCheckResponse is uppercase, so to align for consistency when getting json from q/health or viewing webpage at q/health-ui
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Let's not merge this right away.

The behavior is completely different from the one in the Agroal extension: https://github.com/quarkusio/quarkus/blob/main/extensions/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/health/DataSourceHealthCheck.java and it would probably be better to have a consistent behavior.

But we need decide which is better :).

/cc @yrodiere @tsegismont

@yrodiere
Copy link
Member

@gsmet I'm not sure how this is usually done, but in any case it seems to make sense to give details about each datasource, even those that are up. So I'd be inclined to do the same with Agroal?

@gsmet
Copy link
Member

gsmet commented Jun 29, 2022

@yrodiere OK. Last question is if we should refer to the default datasource as default or <default> for the health checks. Given they are used programmatically, I would lean towards the latter but open to discussion.

@yrodiere
Copy link
Member

OK. Last question is if we should refer to the default datasource as default or <default>

+1 for <default>, that's more consistent with configuration, logs, etc. And it will avoid problems when someone decides to name their datasource default (no </>)...

I created a ticket: #26431

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

OK, then this one is all good. Let's merge it.

@gsmet gsmet merged commit c8c9f44 into quarkusio:main Jun 29, 2022
@quarkus-bot quarkus-bot bot added this to the 2.11 - main milestone Jun 29, 2022
@gsmet
Copy link
Member

gsmet commented Jun 29, 2022

Thanks!

@luckyguy73 luckyguy73 deleted the patch-1 branch June 29, 2022 15:25
@gsmet gsmet changed the title Update ReactiveDatasourceHealthCheck.java Use uppercase UP in ReactiveDatasourceHealthCheck.java Jul 4, 2022
@gsmet gsmet changed the title Use uppercase UP in ReactiveDatasourceHealthCheck.java Use uppercase UP in ReactiveDatasourceHealthCheck Jul 4, 2022
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.

None yet

4 participants