Issue-Fix-#2665 feat: added implementation for explicit path for readiness and liveness Probes #3036
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ss probes Signed-off-by: l3002 <[email protected]>
Eclipse JKube CI ReportStarted new GH workflow run for #3036 (2024-05-09T12:35:48Z) ⚙️ JKube E2E Tests (9017189212)
|
|
@rohanKanojia / @manusa : I need to know the significance of the following line of code before you merge this or take any further action over this PR: I understand there here we are defining what is needed to be returned when The reason I wanted to know this is because while writing the test And that was the reason why I omitted the given lines of code from this test.
Kindly find the below screenshot for the error received: |
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3036 +/- ##
=============================================
+ Coverage 59.36% 70.86% +11.50%
- Complexity 4586 5086 +500
=============================================
Files 500 490 -10
Lines 21211 19599 -1612
Branches 2830 2529 -301
=============================================
+ Hits 12591 13889 +1298
+ Misses 7370 4480 -2890
+ Partials 1250 1230 -20 ☔ View full report in Codecov by Sentry. |
This code seems to be adding a mock for project's class loader. While generating Spring Boot Health Checks we first check whether user has enabled use of health checks by searching for specific classes in classpath
You're right. If we're already mocking |
| writeProps(); | ||
|
|
||
| Probe readinessProbe = new SpringBootHealthCheckEnricher(context).buildProbe(10, null, null, 3, 1,"/example"); | ||
| assertHTTPGetPathAndPort(readinessProbe,getActuatorDefaultBasePath() + "/health", 8080); |
There was a problem hiding this comment.
I think there are tests already to cover the scenario where management.health.probes.enabled is false (I think zeroConfig() has same expectations as this test)
There was a problem hiding this comment.
I thought maybe it would be better to have separate test for the case where the management probe is explicitly kept as false. If you think there's no need for such a testcase, then I can surely remove it.
There was a problem hiding this comment.
Umm, you do have a point. It might make sense to keep these tests in case we refactor zeroConfig() test in future. Having more tests doesn't do any harm.
|
I think we should also add a Gradle Integration test for this feature (like existing spring-boot-properties integration test) . Check this PR to get an idea of how to write gradle integration test #2533 . |
|
Documentation also needs to be updated. Documentation for enricher is here We should inform the user that enabling property would affect liveness and readiness probes. |
|
@rohanKanojia : Thanks for the code review, I'll work on the mentioned pointer over this week. |
|
@rohanKanojia: It might take me some time to write the integration test for this change, but I'll be completing this by the end of next week. Just wanted to let you know. |
|
@l3002 : On second thought, Can we do that in follow-up PRs? Maybe we can merge this one and add some subtasks to the issue regarding what is left to do? |
|
@rohanKanojia: Yeah, that would work as well, moreover this PR has been open for quite some time. |
rohanKanojia
approved these changes
Jun 6, 2024
|
Let's get this one merged, I'll create follow up tasks once this gets merged. |
manusa
added a commit
to manusa/jkube
that referenced
this pull request
Jun 7, 2024
…kube#2665 Signed-off-by: Marc Nuri <[email protected]>
manusa
added a commit
that referenced
this pull request
Jun 7, 2024
Signed-off-by: Marc Nuri <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2665
Added implementation for creating explicit path creation for
readinessandlivenessprobes whenmanagement.health.probes.enabledistrue.Modified
SpringBootHealthCheckEnricherto check if the suffix needs to be concatenated to the paths or not.Created unit tests for the added implementation and verified if everything was working as expected.
Type of change
test, version modification, documentation, etc.)
Checklist