-
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 it part 15 #1486
Fabric8 it part 15 #1486
Conversation
<module>spring-cloud-kubernetes-fabric8-istio-it</module> | ||
|
||
<module>spring-cloud-kubernetes-client-loadbalancer-it</module> | ||
<module>spring-cloud-kubernetes-configuration-watcher-it</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.
there is more then one thing going on in this PR, I'll try to explain everything. The first change is to rename the module, so it becomes on par with the previous changes we made. This is the simplest change here
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.
The other change (there is a longer comment in ActuatorRefreshKafkaIT
), is that we can drop two integration tests, they are not needed anymore, their logical testing is done elsewhere already
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 also refactored two existing tests so that they are faster
/** | ||
* @author Kris Iyer | ||
*/ | ||
class ActuatorRefreshKafkaIT { |
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 already have an integration test that deals with Kafka + configuration-watcher. Unlike this one, it is more involved. It makes sure kafka works with configuration watcher, plus it also tests the fact that multiple pass can be restarted, not a single one. The only reason we have them both, is that this one was written before I wrote the other one.
As such, these tests, do logically the same thing, but one is more involved. We do not really need this one, or the rabbitmq one (for the same reasons). These two tests removal also mean that we can remove a lot of files that they required, thus more files are deleted.
@@ -69,34 +69,6 @@ | |||
</resources> | |||
|
|||
<plugins> | |||
<!-- build image in the 'package' phase, and ignore plain tests --> |
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 do not need to build an image anymore, since everything is asserted against wiremock.
@@ -57,6 +54,9 @@ class ActuatorRefreshIT { | |||
|
|||
private static final String NAMESPACE = "default"; | |||
|
|||
private static final String DOCKER_IMAGE = "docker.io/springcloud/" + SPRING_CLOUD_K8S_CONFIG_WATCHER_APP_NAME + ":" |
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.
there used to be two tests in this class, I refactored so that it is only one and use the same PATCH
for the other one. Logically, nothing has changed
@@ -72,30 +71,22 @@ static void beforeAll() throws Exception { | |||
util = new Util(K3S); |
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.
this class is even easier to simplify, since it does not require PATCH
at all
@ryanjbaxter ready to be looked at, thank you |
friendly reminder... |
No description provided.