-
Notifications
You must be signed in to change notification settings - Fork 219
Conversation
103db9c
to
940b11c
Compare
rktbot run e2e |
1 similar comment
rktbot run e2e |
rktbot run e2e |
rktbot run e2e |
Tests are passing. The most recent pluton run had a flake, but the tests that actually run the code in this PR passed twice in a row now. |
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.
Looks good, just a few questions, nits and such.
if err := retry(15, 10*time.Second, podsReady); err != nil { | ||
return fmt.Errorf("Waited 150 seconds for etcd to scale: %v", err) | ||
if err := retry(31, 10*time.Second, podsReady); err != nil { | ||
return fmt.Errorf("Waited 300 seconds for etcd to scale: %v", err) |
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.
Would be 310
, no?
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.
You would think, but the retry function actually will do N retries and N-1 pauses. So try -> pause -> try -> pause -> try
It doesn't pause after the last try.
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, got it. Thanks.
role = "${aws_iam_role.bk_role.id}" | ||
|
||
provisioner "local-exec" { | ||
command = "sleep 90" |
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 do we need this sleep?
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 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.
👍 -- would you mind throwing in a link to that issue?
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 included it in the commit, adding it to PR text as well
e2e/deleteapi_test.go
Outdated
// apiserver to go down the next step will return sucess before the | ||
// apiserver is ever destroyed. | ||
waitDestroy := func() error { | ||
_, err := client.CoreV1().Pods(namespace).List(metav1.ListOptions{LabelSelector: "k8s-app=kube-apiserver"}) |
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.
nit: Could just do a simpler request to the version endpoint or something, if you're just checking that the API server is up and reachable.
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'm fine with this. Offhand do you know the client-go function to call for this (not obvious to me with a bit of searching other then using the rest-client for which the error handling looks weirder)
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.
done
e2e/deleteapi_test.go
Outdated
|
||
// wait until api server is back up | ||
waitAPI := func() error { | ||
_, err := client.CoreV1().Pods(namespace).List(metav1.ListOptions{LabelSelector: "k8s-app=kube-apiserver"}) |
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.
Same nit.
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.
done
rktbot run e2e |
@derekparker PTAL should be ready to go. |
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.
lgtm
Also adds a work-around for hashicorp/terraform#1885