Improved guestbook.sh test to become actual test #5294
Conversation
|
Satnam, could you please take a look? Should be merged after #5284 |
|
Happy to review although I always value @filbranden 's words of wisdom about bash. I will try to get to this after the Fixit today -- which might mean no action until tomorrow. Are you in a hurry? Perhaps not if you are blocked on #5248 |
| fi | ||
| } | ||
|
|
||
| function wait_for_running() { | ||
| echo "Waiting for pods to come up." | ||
| frontends=$(${KUBECTL} get pods -l name=frontend -o template '--template={{range.items}}{{.id}} {{end}}') |
There was a problem hiding this comment.
Declare these variables as local and do it in its own line (no assignment to $(...) on the same line as local as that hides the error exit of the command.)
local frontends master slaves pods all_running status i pod
frontends=$(...)
...
|
Handing over to @filbranden who is clearly in charge here. |
| pods=$(echo $frontends $master $slaves) | ||
|
|
||
| all_running=0 | ||
| for i in $(seq 1 30); do |
There was a problem hiding this comment.
While $(seq ...) is just fine, bash can do it with a built-in for you...
for i in {1..30}; do
|
Looks good overall! Thanks @piosz for tackling this one! I have a lot of notes (as usual) but in general the code looks good and there's nothing structural, only style and nitpicks. Please let me know when ready to take another look, I'll review it and merge it when it's ready. Cheers! |
After change test waits until pods come up and frotend starts serving content, then adds entry to guestbook and verify if operation succeed This fixes kubernetes#3693
|
Thanks for comments. I resolved them (hopefully all). Sorry for such bad style but it's the first time I'm coding in bash;) PTAL BTW do you prefer updated version as a new commit or amend to the previous one (which I did this time)? |
|
Looks great. ammended commit is fine, it's a bit annoying to have to review everything again but in the end it works just fine. (If you do the other way around I'd ask you to squash it before we merge it.) Merging now. Don't worry, your bash was great... way above average. The problem is the extra nitpicky reviewer ;-) Cheers! |
Improved guestbook.sh test to become actual test
|
Thanks! |
satnam6502
added
the
lgtm
After change the test waits until pods come up and frontend starts serving content,
then adds entry to guestbook and verify if operation succeed
This fixes #3693