Skip to content
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

force pull fixes / debug #8562

Merged
merged 1 commit into from Apr 20, 2016
Merged

Conversation

gabemontero
Copy link
Contributor

@bparees @csrwng PTAL ...

@bparees
Copy link
Contributor

bparees commented Apr 19, 2016

lgtm but i see the test failed, do you want to investigate that further before this gets merged?

@gabemontero
Copy link
Contributor Author

Similar to another PR I just attempted an extended test on, this extended test failed with a vagrant / AWS hiccup: not enough capacity in the US east zone, prior to the extended test running.

That said, I'd still like to see a pre-merge run with the changes in the PR.

@@ -30,6 +30,21 @@ func ResetImage(tags map[string]string) {

}

//DumpImage is a helper that inspects the image along with some ginkgo debug
func DumpImage(name string) {
g.By(fmt.Sprintf("Calling docker inspect for image %s", name))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think that e2e.Logf would be more appropriate here.
g.By is used to describe test cases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or directly fmt.Fprintf() to g.GinkgoWriter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

went with the latter (even got some nice blue highlighting in emacs with it :-) ) ... thanks - will push in a moment

@gabemontero
Copy link
Contributor Author

Somehow "forcePull" got converted to "forcepull" and the extended test failed with not being able to find the custom focus....I'll see if I can find some sort of logic like that in the vagrant plugin.

@gabemontero
Copy link
Contributor Author

[testonlyextended][extended:core(builds)]

@gabemontero
Copy link
Contributor Author

@bparees - i ran the full builds suite given the forcepull/forcePull conundrum (I decided not the change the "forcePull" string in the Describe to "forcepull" at this time), and the forcePull test passed.

Please hit the merge button unless you've had a change of opinion from earlier.

@bparees
Copy link
Contributor

bparees commented Apr 19, 2016

the other g.By() invocations should probably be converted as well.

@gabemontero
Copy link
Contributor Author

gabemontero commented Apr 20, 2016

other newly added g.By() invocations converted and pushed

@gabemontero
Copy link
Contributor Author

[testonlyextended][extended:core(builds)]

@openshift-bot
Copy link
Contributor

Evaluated for origin testonlyextended up to a39d507

@bparees
Copy link
Contributor

bparees commented Apr 20, 2016

lgtm but needs secondary merge approval. @smarterclayton this only affects extended tests, no product code.

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testonlyextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/72/) (Extended Tests: core(builds))

@smarterclayton
Copy link
Contributor

Approved

On Apr 19, 2016, at 10:30 PM, Ben Parees notifications@github.com wrote:

lgtm but needs secondary merge approval. @smarterclayton
https://github.com/smarterclayton this only affects extended tests, no
product code.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8562 (comment)

@bparees
Copy link
Contributor

bparees commented Apr 20, 2016

[merge]

Ben Parees | OpenShift
On Apr 19, 2016 23:19, "Clayton Coleman" notifications@github.com wrote:

Approved

On Apr 19, 2016, at 10:30 PM, Ben Parees notifications@github.com wrote:

lgtm but needs secondary merge approval. @smarterclayton
https://github.com/smarterclayton this only affects extended tests, no
product code.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8562 (comment)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8562 (comment)

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5646/) (Image: devenv-rhel7_4003)

@bparees
Copy link
Contributor

bparees commented Apr 20, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to a39d507

@openshift-bot openshift-bot merged commit 3b1e43d into openshift:master Apr 20, 2016
@gabemontero gabemontero deleted the fixFPExtTest branch April 20, 2016 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants