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

[issue-325] - Add tests to verify that HealthCHeckResponse has a name when built as recommended on the producer side #326

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fabiobrz
Copy link

SSIA.

Fixes #325

Draft - still running TCKs

Copy link
Member

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

Thanks @fabiobrz for your PR. But I need to request some changes. I don't think generating the name should be our solution to null names. I don't see the point of having random name since it won't (probably) give value to user when the health checks is first invoked. So then when then need to restart the app to add the name, I think it would be better to throw and exception (possibly on startup when we detect null name) to prevent app from running.

Also in every case, this will require adjustment in the specification text. I'm willing to hop on a quick call if you want to discuss this case in more detail. (/cc @pgunapal I would like to hear you opinion too).

@fabiobrz
Copy link
Author

Thanks @fabiobrz for your PR. But I need to request some changes. I don't think generating the name should be our solution to null names. I don't see the point of having random name since it won't (probably) give value to user when the health checks is first invoked. So then when then need to restart the app to add the name, I think it would be better to throw and exception (possibly on startup when we detect null name) to prevent app from running.

Yeah, you might be right in here, thanks, fixing.

Also in every case, this will require adjustment in the specification text. I'm willing to hop on a quick call if you want to discuss this case in more detail. (/cc @pgunapal I would like to hear you opinion too).

Correct, the spec text should be adjusted. I am glad to learn about this, so feel free to get in touch, and thanks for your review!

fabiobrz pushed a commit to fabiobrz/smallrye-health that referenced this pull request Mar 13, 2024
…own when HealthCheckResponse name is null or empty string
@fabiobrz fabiobrz changed the title [issue-325] - Make sure HealthCheckResponse is given a name when invoked through ::ctor, ::up, and ::down [issue-325] - Add tests to verify that HealthCHeckResponse has a name when built as recommended on the producer side Mar 15, 2024
@pgunapal
Copy link
Contributor

I agree with @xstefank, It wouldn't make sense to randomly generate a name for a Health check with no-name.
We should throw an exception if we detect a health check without a name.

+1 on updating the specification, with this detail.

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.

HealthCheckResponse should enforce that the name cannot be null
3 participants