Application image test rework#225
Conversation
|
@skoshchi Is it worth creating a separate function to wait for the "ACTIVE" state of all pods? Yes Otherwise the code looks OK |
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
from
2809157 to
38087e8
Compare
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
3 times, most recently
from
a52d443 to
567c24e
Compare
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
2 times, most recently
from
25a8660 to
f0ff8db
Compare
skoshchi
changed the title
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
from
ae61587 to
e79247a
Compare
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
from
e79247a to
8af29c9
Compare
|
|
||
| foundImage := foundDeployment.Spec.Template.Spec.Containers[0].Image | ||
| if foundImage != newImage { | ||
| if foundImage != testImgOld { |
There was a problem hiding this comment.
This looks like a logic mistake. Previous version compared the found image with the image which was set on line 83. You are comparing it with different image.
| return true | ||
| }, time.Second*30, time.Millisecond*250).Should(BeTrue()) | ||
|
|
||
| waitForPodsActiveState(name) |
There was a problem hiding this comment.
What is the difference between this check and check on the line 111?
| Testing can be configured via environment variables: | ||
| - NAMESPACE_FOR_TESTING - Namespace where webservers will be deployed. Default value: ```jws-operator-tests``` | ||
| - TEST_IMG - Default image for tests which do not require specific image. Default value: ```quay.io/web-servers/tomcat-demo``` | ||
| - TEST_IMG_OLD - The image required for the Update test. Default value: ```quay.io/web-servers/tomcat10update:latest``` |
There was a problem hiding this comment.
I don't like the name TEST_IMG_OLD. What about TEST_IMAGE_FOR_UPDATE or TEST_IMAGE_II with description: Additional/The second test image which is used in update test.
Or remove this settings and keep the tomcat10update image hard coded. The test just switches one image to the other and it doesn't matter what images it uses. It's only about whether the change happens.
| }, time.Minute*5, time.Second*5).Should(BeTrue(), "Building pods took too long time.") | ||
| } | ||
|
|
||
| func waitForPodsActiveState(name string) { |
There was a problem hiding this comment.
I have a problem with this function.
It succeeds when number of pods from operator status is the same as number of replicas set in the operator and all the pods are in active state. What if there are pods in terminating state?
What if the status of the operator does not work correctly, wouldn't be better to check the number of pods from deployment of directly from the cluster?
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
from
8af29c9 to
e0091a2
Compare
skoshchi
force-pushed
the
ApplicationImageTestStability
branch
from
e0091a2 to
6e1c92f
Compare
2 participants
Is it worth creating a separate function to wait for the "ACTIVE" state of all pods? This error appears only once. Can we have something similar in the future?