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

enable builds with PR references #13893

Merged
merged 1 commit into from
Apr 28, 2017

Conversation

gabemontero
Copy link
Contributor

@gabemontero
Copy link
Contributor Author

@gabemontero
Copy link
Contributor Author

[testextended][extended:core(should start a build from a PR ref)]

@gabemontero
Copy link
Contributor Author

[test]

@@ -252,7 +252,16 @@ func extractGitSource(gitClient GitClient, gitSource *api.GitBuildSource, revisi
}

if err := gitClient.Checkout(dir, commit); err != nil {
return true, err
//TODO - do we want to parse the error string for the text that accompanies a clone / checkout of a PR ref, such as: pathspec 'refs/pull/1/head' did not match any file(s) known to git
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this case is infrequent enough that it's not worth introducing the fragility that would come w/ that optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good ... I'll remove the TODO comment

@gabemontero
Copy link
Contributor Author

On the 1 test failure, we hit flake #12072

@@ -252,7 +252,16 @@ func extractGitSource(gitClient GitClient, gitSource *api.GitBuildSource, revisi
}

if err := gitClient.Checkout(dir, commit); err != nil {
return true, err
//TODO - do we want to parse the error string for the text that accompanies a clone / checkout of a PR ref, such as: pathspec 'refs/pull/1/head' did not match any file(s) known to git
err = gitClient.Fetch(dir, gitSource.URI, commit)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's probably worth adding some level 4 logging to this code path so we know when we've gone down it successfully (or if we ever go down it incorrectly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

part of next push

@@ -51,6 +52,38 @@ var _ = g.Describe("[builds][Slow] starting a build using CLI", func() {
})
})

g.Describe("oc start-build with pr ref", func() {
g.It("should start a build from a PR ref, wait for the build to complete, and confirm the right level was used", func() {
g.By("make sure wildly is has latest tag")
Copy link
Contributor

Choose a reason for hiding this comment

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

s/is/imagestream/ (i read "is" as the verb..it was very confusing :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

part of next push

break
}
time.Sleep(100 * time.Millisecond)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wait.Poll()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out there already is a helper function for this ... will switch to that.

@gabemontero
Copy link
Contributor Author

updates pushed @bparees - thanks

@@ -252,7 +252,17 @@ func extractGitSource(gitClient GitClient, gitSource *api.GitBuildSource, revisi
}

if err := gitClient.Checkout(dir, commit); err != nil {
return true, err
glog.V(4).Infof("Checkout after clone failed for ref %s with error: %#v, attempting fetch", commit, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/%#v/%v/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just making sure @bparees - you decided you were OK with this and you posted the merge comment ?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, apparently I forgot :) merge comment deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update pushed

@bparees
Copy link
Contributor

bparees commented Apr 25, 2017

one last nit and lgtm.

@gabemontero
Copy link
Contributor Author

second test run hit flake #13650

@gabemontero
Copy link
Contributor Author

nit addressed

@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to 1be7025

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 1be7025

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/232/) (Base Commit: 60180d1) (Extended Tests: core(should start a build from a PR ref))

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/943/) (Base Commit: 60180d1)

@smarterclayton
Copy link
Contributor

Sweet, want this for our own CI

@bparees
Copy link
Contributor

bparees commented Apr 26, 2017

Sweet, want this for our own CI

we did it just for you. 🎁

@jim-minter
Copy link
Contributor

nice!

@gabemontero
Copy link
Contributor Author

On the third test run, we hit flake #13650 again, as well as a network/env hiccup during the network minimal test:

Error: Error downloading packages:
  Curl error (35): SSL connect error for https://mirrors.fedoraproject.org/metalink?repo=fedora-25&arch=x86_64 [Encountered end of file]
The command '/bin/sh -c dnf -y update && dnf -y install bind-utils findutils hostname iproute iputils less procps-ng tar which bridge-utils ethtool iptables-services openvswitch && dnf clean all' returned a non-zero code: 1
[ERROR] PID 13696: hack/dind-cluster.sh:276: `${DOCKER_CMD} build -t "${image_name}" .` exited with status 1.

@bparees - I think we are good for merge .... I saw some conversation in other PRs about whether it was valid to post the merge comment while things were on hold for the rebase, but I don't know if I saw a conclusion as to what was valid. So please post/not post as needed. Thanks.

@bparees
Copy link
Contributor

bparees commented Apr 26, 2017

[merge] (queue is disabled but will merge once it's back up)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 1be7025

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 28, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/498/) (Base Commit: 325c02f) (Image: devenv-rhel7_6184)

@openshift-bot openshift-bot merged commit 5f212b0 into openshift:master Apr 28, 2017
@gabemontero gabemontero deleted the fetch-git-pr branch April 28, 2017 13:41
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.

5 participants