-
Notifications
You must be signed in to change notification settings - Fork 469
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
feat (jkube-kit/wildfly-jar): Add startup probe in WildFlyJARHealthCheckEnricher (#1474) #1492
Conversation
Eclipse JKube CI ReportStarted new GH workflow run for #1492 (2022-08-24T06:12:29Z) ⚙️ JKube E2E Tests (2916741610)
|
...-jar/src/main/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricher.java
Outdated
Show resolved
Hide resolved
...-jar/src/main/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricher.java
Outdated
Show resolved
Hide resolved
63a9700
to
afea257
Compare
Codecov Report
@@ Coverage Diff @@
## master #1492 +/- ##
=========================================
Coverage 52.76% 52.76%
+ Complexity 3932 3925 -7
=========================================
Files 461 462 +1
Lines 20752 20754 +2
Branches 2819 2817 -2
=========================================
+ Hits 10949 10951 +2
Misses 8681 8681
Partials 1122 1122
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
d164cd6
to
2dd8a4c
Compare
Could you please rebase your branch against master? |
Sure |
2dd8a4c
to
8b431ba
Compare
8b431ba
to
4860979
Compare
I think we would need to add startup probes only when the underlying framework supports it (just like we did in case of Quarkus in #1524 ). We would need to check whether Wildfly version is at least |
7233694
to
a716c37
Compare
Kudos, SonarCloud Quality Gate passed! |
a716c37
to
f4757fb
Compare
...t/jkube-kit-wildfly-jar/src/test/java/org/eclipse/jkube/wildfly/jar/WildflyJarUtilsTest.java
Outdated
Show resolved
Hide resolved
...t/jkube-kit-wildfly-jar/src/test/java/org/eclipse/jkube/wildfly/jar/WildflyJarUtilsTest.java
Outdated
Show resolved
Hide resolved
...t/jkube-kit-wildfly-jar/src/test/java/org/eclipse/jkube/wildfly/jar/WildflyJarUtilsTest.java
Outdated
Show resolved
Hide resolved
...t/jkube-kit-wildfly-jar/src/test/java/org/eclipse/jkube/wildfly/jar/WildflyJarUtilsTest.java
Outdated
Show resolved
Hide resolved
...-jar/src/main/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricher.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricherTest.java
Outdated
Show resolved
Hide resolved
f4757fb
to
d8899f2
Compare
.../src/test/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricherTest.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricherTest.java
Outdated
Show resolved
Hide resolved
assertThat(probe.getInitialDelaySeconds().intValue()).isEqualTo(99); | ||
assertThat(probe.getPeriodSeconds().intValue()).isEqualTo(10); | ||
assertThat(probe.getSuccessThreshold().intValue()).isEqualTo(27); | ||
assertThat(probe.getFailureThreshold().intValue()).isEqualTo(17); |
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.
I see there are a lot of places where we're asserting Probe
contents. Do you think we should create a new method assertProbe
that would receive probe to verify and expected values?
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.
It would be nice as it was done in Quarkus Test
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.
Thanks a lot 🙏 .
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.
Should we do something like this?
private void assertProbe(Probe probe, String scheme, int port, String path, int initialDelaySeconds,
int periodSeconds, int successThreshold, int failureThreshold) {
assertThat(probe)
.isNotNull()
.hasFieldOrPropertyWithValue("httpGet.host", null)
.hasFieldOrPropertyWithValue("httpGet.scheme", scheme)
.hasFieldOrPropertyWithValue("httpGet.port", new IntOrString(port))
.hasFieldOrPropertyWithValue("httpGet.path", path)
.hasFieldOrPropertyWithValue("initialDelaySeconds", initialDelaySeconds)
.hasFieldOrPropertyWithValue("periodSeconds", periodSeconds)
.hasFieldOrPropertyWithValue("successThreshold", successThreshold)
.hasFieldOrPropertyWithValue("failureThreshold", failureThreshold);
}
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.
looks good, but I would move httpGet related assertions to another method assertHttpGet
and use that here. I see some places where we only check httpGet. This method could be reused there as well.
d8899f2
to
5506ff9
Compare
Nice work! |
5506ff9
to
a1cad77
Compare
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.
Looking good, but there are a few things to be changed
...-jar/src/main/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricher.java
Outdated
Show resolved
Hide resolved
...-jar/src/main/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricher.java
Outdated
Show resolved
Hide resolved
...-jar/src/main/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricher.java
Outdated
Show resolved
Hide resolved
...-jar/src/main/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricher.java
Outdated
Show resolved
Hide resolved
...-jar/src/main/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricher.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricherTest.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricherTest.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricherTest.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricherTest.java
Outdated
Show resolved
Hide resolved
...ldfly-jar/src/test/java/org/eclipse/jkube/wildfly/jar/generator/WildflyJARGeneratorTest.java
Outdated
Show resolved
Hide resolved
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.
I see there are still many pending changes.
What's your status on this?
a1cad77
to
b6ba6da
Compare
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.
We're almost there.
...-jar/src/main/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricher.java
Outdated
Show resolved
Hide resolved
...t-wildfly-jar/src/main/java/org/eclipse/jkube/wildfly/jar/generator/WildflyJARGenerator.java
Outdated
Show resolved
Hide resolved
...t-wildfly-jar/src/main/java/org/eclipse/jkube/wildfly/jar/generator/WildflyJARGenerator.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/eclipse/jkube/wildfly/jar/enricher/WildflyJARHealthCheckEnricherTest.java
Outdated
Show resolved
Hide resolved
b6ba6da
to
709dea3
Compare
709dea3
to
aa20357
Compare
…eckEnricher (eclipse-jkube#1474) Wildfly supports startup endpoint since '25.0.0.Final'. Add a check to only add startup probes in projects using Wildfly 25.0.x and later Signed-off-by: Anurag Rajawat <anuragsinghrajawat22@gmail.com>
aa20357
to
efad820
Compare
Kudos, SonarCloud Quality Gate passed! |
@manusa please review this PR |
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, thx!
@manusa sorry for the noise and thanks a lot for helping me. |
Description
Fix: #1474
Signed-off-by: Anurag Singh Rajawat anuragsinghrajawat22@gmail.com
Type of change
test, version modification, documentation, etc.)
Checklist