-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fabric8 configmap event reload patch refactor part 3 #1467
Fabric8 configmap event reload patch refactor part 3 #1467
Conversation
…nto fabric8-configmap-event-reload-patch-refactor-part-3
@@ -52,9 +52,8 @@ | |||
<module>spring-cloud-kubernetes-fabric8-istio-it</module> | |||
<module>spring-cloud-kubernetes-fabric8-client-discovery</module> | |||
<module>spring-cloud-kubernetes-fabric8-client-loadbalancer</module> | |||
<module>spring-cloud-kubernetes-fabric8-client-configmap-polling-reload</module> |
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 think this is the easiest way to look at this PR: I drop spring-cloud-kubernetes-fabric8-client-configmap-polling-reload
and refactor all of its tests by placing them into spring-cloud-kubernetes-fabric8-client-reload
.
Logically, the tests do not change at all, just merge them in a different project and use PATCH
like testing for them
@@ -139,6 +144,10 @@ void testInformFromOneNamespaceEventNotTriggered() { | |||
testInformFromOneNamespaceEventTriggered(); | |||
testInform(); | |||
testInformFromOneNamespaceEventTriggeredSecretsDisabled(); | |||
testDataChangesInConfigMap(); |
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.
all methods that were in the spring-cloud-kubernetes-fabric8-client-configmap-polling-reload
were ported in this test now
@@ -116,7 +116,7 @@ public static void assertReloadLogStatements(String left, String right, String a | |||
LOG.info("appPodName : ->" + appPodName + "<-"); | |||
// we issue a pollDelay to let the logs sync in, otherwise the results are not | |||
// going to be correctly asserted | |||
await().pollDelay(20, TimeUnit.SECONDS).pollInterval(Duration.ofSeconds(5)).atMost(Duration.ofSeconds(600)) |
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.
lower the interval from 600 to 120
@ryanjbaxter ready to be looked at. thank you |
Commons.waitForLogStatement("will add file-based property source : /tmp/application.properties", container, | ||
appLabelValue); | ||
// (3) | ||
WebClient webClient = builder().baseUrl("http://localhost/key").build(); |
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.
Why use WebClient instad of RestClient?
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.
all of our integration tests are using WebClient... not exactly sure what you are trying to suggest tbh
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.
Huh, I guess I never noticed. It just seemed odd to me to use WebClient and then block when you could just use RestTemplate
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.
huh! good point :) I think that initially we used it because of RestTemplate deprecation, but now that you put it like this, I can't recall the exact history behind it
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.
RestTemplate isn't going anywhere. Anyways it is no big deal, its a test
@@ -26,9 +26,12 @@ | |||
<artifactId>spring-boot-starter-actuator</artifactId> | |||
</dependency> | |||
|
|||
<!-- not bootstrap starter, because we want to be able to disable bootstrap per-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.
Where are the tests that are not using bootstrap?
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.
ConfigMapPollingReloadDelegate.testConfigMapPollingReload
and
ConfigMapMountPollingReloadDelegate.testConfigMapMountPollingReload
The way to find out this info after our refactoring is to look where the body is for the PATCH and there will be an environment variable in the form:
{
"name": "SPRING_CLOUD_BOOTSTRAP_ENABLED",
"value": "FALSE"
}
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.
Now I see things clearer. For some reason before I was just looking at a single commit and not all the changes. Sorry for the dumb question.
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.
ah, no! every question we discuss closes a few gaps here and there.
No description provided.