Conversation
65e0333 to
cccc80c
Compare
joestringer
requested changes
Aug 28, 2020
christarazi
requested changes
Aug 28, 2020
Member
christarazi
left a comment
There was a problem hiding this comment.
Nice find! A few minor changes, and it should be good to go.
cccc80c to
ce73840
Compare
christarazi
approved these changes
Aug 28, 2020
ce73840 to
e7b8243
Compare
aanm
approved these changes
Sep 1, 2020
| - name: redis-master | ||
| image: docker.io/library/redis:4.0.11 | ||
| - name: leader | ||
| image: "docker.io/redis:6.0.5" |
Member
Author
There was a problem hiding this comment.
No preference either way, but trying to understand as Google takes different convention than you: why do you prefer the docker.io/library/xxx form?
e7b8243 to
24ce546
Compare
ae9e4be updated the GuestBook images and labels, but failed to make the same label update in the test itself. Thus, since then, we have not been running any connectivity check in the GuestBook test. That went unnoticed because we didn't check that the set of pods returned (from which we run connectivity checks) was not empty. This commit fixes it by: 1. Updating the label in the test itself to app=guestbook. 2. Adding a check that the set of pods selected isn't empty. However, the nc utility we were using to check connectivity from the frontend pods to the Redis backend isn't available in the new images. Therefore, we also need to: 3. Use curl instead inside the frontend pods to check that the PHP frontend works as expected and is able to contact the Redis backend. That's it? No. Turns out some of the pod labels and names have also been hardcoded in the Docker images and have been updated (mostly to use more neutral terms). 4. Update the YAML file to better match [1]. We however can't update the 'redis-master' name because our v6 frontend image has it hardcoded. The v5 frontend image at [1] has 'redis-leader' as the name, but somehow not the v6. We want to use the v6 image because it is a lot bigger (cf. dffb450). 5. And finally, Bob's our uncle! 1 - https://cloud.google.com/kubernetes-engine/docs/tutorials/guestbook Fixes: #12994 Fixes: ae9e4be ("test: replace guestbook test docker image") Signed-off-by: Paul Chaignon <paul@cilium.io>
This reverts commit c84d0a2. This test has been fixed by 240d808 ("test: Fix GuestBook test"). Signed-off-by: Paul Chaignon <paul@cilium.io>
24ce546 to
815cc0f
Compare
Member
Author
|
test-me-please |
joestringer
approved these changes
Sep 2, 2020
Member
joestringer
left a comment
There was a problem hiding this comment.
I think it's a great question what we gain from this guestbook test that's not otherwise covered elsewhere in the CI. Maybe just to ensure we have a full-on end-to-end test?
Member
Author
@joestringer Yeah, but I don't think it's our only end-to-end test of that sort (e.g., Star wars demo). I've added a TODO note to double check these to see if there's actually any meaningful difference.
|
pchaigno
added a commit
to cilium/packer-ci-build
that referenced
this pull request
Sep 3, 2020
to match cilium/cilium#13003. Signed-off-by: Paul Chaignon <paul@cilium.io>
nebril
pushed a commit
to cilium/packer-ci-build
that referenced
this pull request
Sep 9, 2020
to match cilium/cilium#13003. Signed-off-by: Paul Chaignon <paul@cilium.io>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
#7955 updated the GuestBook images and labels, but failed to make the same label update in the test itself. Thus, since then, we have not been running any connectivity check in the GuestBook test. That went unnoticed because we didn't check that the set of pods returned (from which we run connectivity checks) was not empty.
This commit fixes it by:
However, the
ncutility we were using to check connectivity from the frontend pods to the Redis backend isn't available in the new images. Therefore, we also need to:That's it? No. Turns out some of the pod labels and names are also hardcoded in the Docker images and have been updated (mostly to use more neutral terms).
1 - https://cloud.google.com/kubernetes-engine/docs/tutorials/guestbook
Fixes: #12994
Fixes: #7955
/cc @aanm
Signed-off-by: Paul Chaignon paul@cilium.io