oc start-build --from-repo now uses clone+checkout #10316

oatmealraisin · 2016-08-09T21:20:54Z

This allows us to handle submodules. However, it takes up a bit more
resources. This shouldn't be a problem - even the Linux kernel only
took about 3 seconds to clone to another local directory.

Fixes #8922
@bparees @sosiouxme ptal?

oatmealraisin · 2016-08-09T21:21:01Z

[test]

bparees · 2016-08-09T21:27:57Z

pkg/cmd/cli/cmd/startbuild.go

+					Recursive: true,
+					Shallow:   false,
+					// TODO: Should we output this at a loglevel?  If so, what level?
+					Quiet: true,


let's say loglevel 3 and above.

oatmealraisin · 2016-08-11T18:04:43Z

So I think (hope) I've proven that the code is safe to delete as is. Using the code in the original as the test case, we can show that this code runs fine.

oc start-build logging-auth-proxy --follow --from-dir /tmp/foo
Uploading directory "/tmp/foo" as binary input for the build ...
logging-auth-proxy-1
warning: OpenShift server version "v1.3.0-alpha.2+f2b7fdd-dirty" differs from this image "v1.3.0-alpha.2"
Receiving source from STDIN as archive ...
Step 1 : FROM library/node:0.10.36
 ---> 600febd9d7b6
Step 2 : MAINTAINER Luke Meyer <[email protected]>
 ---> Using cache
 ---> d980df35d93f
Step 3 : ENTRYPOINT openshift-auth-proxy
 ---> Using cache
 ---> 3396f4e65132
Step 4 : COPY . /opt/openshift-auth-proxy
 ---> Using cache
 ---> 824a983cf3a7
Step 5 : RUN cd /opt/openshift-auth-proxy     && npm install -g .
 ---> Using cache
 ---> 649992ed0d05
Step 6 : ENV "OPENSHIFT_BUILD_NAME" "logging-auth-proxy-1" "OPENSHIFT_BUILD_NAMESPACE" "default"
 ---> Using cache
 ---> 5a874cec0111
Step 7 : LABEL "io.openshift.build.source-context-dir" "deployer/common/openshift-auth-proxy"
 ---> Using cache
 ---> c9e91c6676a7
Successfully built c9e91c6676a7
Pushing image 172.30.154.146:5000/default/logging-auth-proxy:latest ...
Push successful

However, it would probably still run fine if there was no code in the directory. So we change the line at 504 to os.RemoveAll(os.Join(tempDirectory, "deployer", "common)), which contains the files that were not being copied originally. Without these, the build would fail.

On running this code, we get the same output as before:

oc start-build logging-auth-proxy --follow --from-dir /tmp/foo
Uploading directory "/tmp/foo" as binary input for the build ...
logging-auth-proxy-1
warning: OpenShift server version "v1.3.0-alpha.2+f2b7fdd-dirty" differs from this image "v1.3.0-alpha.2"
Receiving source from STDIN as archive ...
Step 1 : FROM library/node:0.10.36
 ---> 600febd9d7b6
Step 2 : MAINTAINER Luke Meyer <[email protected]>
 ---> Using cache
 ---> d980df35d93f
Step 3 : ENTRYPOINT openshift-auth-proxy
 ---> Using cache
 ---> 3396f4e65132
Step 4 : COPY . /opt/openshift-auth-proxy
 ---> Using cache
 ---> 824a983cf3a7
Step 5 : RUN cd /opt/openshift-auth-proxy     && npm install -g .
 ---> Using cache
 ---> 649992ed0d05
Step 6 : ENV "OPENSHIFT_BUILD_NAME" "logging-auth-proxy-1" "OPENSHIFT_BUILD_NAMESPACE" "default"
 ---> Using cache
 ---> 5a874cec0111
Step 7 : LABEL "io.openshift.build.source-context-dir" "deployer/common/openshift-auth-proxy"
 ---> Using cache
 ---> c9e91c6676a7
Successfully built c9e91c6676a7
Pushing image 172.30.154.146:5000/default/logging-auth-proxy:latest ...
Push successful

Great, but it still may be empty and succeeding. So we move line 504 to 532, immediately after the clone, before the tar.

Running this code, we get a failure:

oc start-build logging-auth-proxy --follow --from-repo /tmp/foo
Uploading "/tmp/foo" at commit "HEAD" as binary input for the build ...
logging-auth-proxy-4
warning: OpenShift server version "v1.3.0-alpha.2+f2b7fdd-dirty" differs from this image "v1.3.0-alpha.2"
Receiving source from STDIN as archive ...
error: build error: open /tmp/docker-build124180944/deployer/common/openshift-auth-proxy/Dockerfile: no such file or directory

This should be because we have deleted the content before the tar-ing takes place.

@bparees ptal?

bparees · 2016-08-11T18:41:48Z

pkg/cmd/cli/cmd/startbuild.go

+					w.Close()
+					pw.CloseWithError(io.EOF)
+				}
+				transferFinished <- false


why not true? (I realize you're doing nothing w/ the value, but true seems more sensible. or better still, the value of "err"

I could change it to true, but the only effect that I wanted was to send a signal through to indicating that a deletion is OK. I don't think we should do error handling on the other side of the channel, because it is specifically for garbage collection.

bparees · 2016-08-11T19:56:42Z

Marking 1.4 because i don't think we want to risk this change in 1.3 at this point.

bparees · 2016-09-20T08:07:00Z

@oatmealraisin needs a rebase

oatmealraisin · 2016-09-20T13:50:46Z

@bparees Rebased

bparees · 2016-09-20T19:34:17Z

pkg/cmd/cli/cmd/startbuild.go

+					select {
+					case <-transferFinished:
+					case <-time.After(timeout):
+						glog.Warning("Transferring directory %q timed out after %q seconds.", clean, timeout.Seconds())


is there really a need for this? the previous iteration of the code didn't worry about the timeout case. can you just move the removeAll down below where the transfer actually happens, with a flag to indicate if you should removeall (because you cloned to a tempdir) or not removeall (because you just uploaded from a dir the user specified)

bparees · 2016-09-20T19:34:58Z

[testextended][extended:core(builds)]

bparees · 2016-09-20T19:35:14Z

[test]

oatmealraisin · 2016-09-21T19:56:33Z

I believe there was a flake, #9548

@bparees I changed to a boolean flag, ptal?

bparees · 2016-09-21T20:30:17Z

the change looks good to me, but you've been victimized by your other pull merging:

++ Building go targets for linux/amd64: cmd/oc
# github.com/openshift/origin/pkg/cmd/cli/cmd
pkg/cmd/cli/cmd/startbuild.go:512: undefined: "github.com/openshift/origin/pkg/generate/git".CloneOptions

so you'll need to rework that code.

oatmealraisin · 2016-09-21T20:32:04Z

@bparees Yep just noticed that, I'm on it

oatmealraisin · 2016-09-22T15:11:02Z

Flake #9548

[test]

oatmealraisin · 2016-09-26T21:07:28Z

Flake #9457, [test]

bparees · 2016-09-28T16:18:46Z

@oatmealraisin pretty sure the extended test failure is caused by these changes, it's not a known flake.

oatmealraisin · 2016-09-28T16:20:18Z

@bparees Yes sorry, I'm working on the Jenkins PR atm

oatmealraisin · 2016-10-18T16:10:36Z

[test]

oatmealraisin · 2016-10-26T14:07:44Z

Flake #11517 , [test]

oatmealraisin · 2016-10-26T20:15:49Z

@bparees I've gotten the extended tests working, ptal?

oatmealraisin · 2016-10-27T19:29:51Z

@bparees The code will search upwards for the root dir. I'm having trouble testing locally, don't know if this will pass, but ptal?

bparees

one nit and lgtm.

bparees · 2016-10-27T20:08:30Z

test/extended/builds/start.go

-			// NOTE: This actually takes the commit from the origin repository. If the
-			// test-build-app changes, this commit has to be bumped.
-			br, err := exutil.StartBuildAndWait(oc, "sample-build", "--commit=f0f3834", fmt.Sprintf("--from-repo=%s", exampleBuild))
+			gitCmd := exec.Command("git", "rev-parse", "HEAD")


do HEAD~1 so we're not just building HEAD. otherwise the git commit matching check later is suspect.

Would a tag be better? That way it doesn't change at all

but we'd be broken if the tag went away for some reason.

also if someone does break this w/ a commit to origin we probably want to know

oatmealraisin · 2016-11-02T01:56:45Z

Flake #11406, [test]

oatmealraisin · 2016-11-03T15:00:38Z

Waiting for Custom Build Strategy fix

bparees · 2016-11-03T16:42:43Z

@oatmealraisin please squash in the meantime.

bparees · 2016-11-03T16:43:17Z

@oatmealraisin i'm comfortable w/ merging this based on the only failure being a known issue, once you squash.

oatmealraisin · 2016-11-03T17:18:37Z

@bparees Roger that, squashing

This allows us to handle submodules. However, it takes up a bit more resources. This shouldn't be a problem - even the Linux kernel only took about 3 seconds to clone to another local directory.

openshift-bot · 2016-11-03T17:22:20Z

Evaluated for origin test up to 5d22f46

openshift-bot · 2016-11-03T17:22:29Z

Evaluated for origin testextended up to 5d22f46

bparees · 2016-11-03T18:01:49Z

[merge]

openshift-bot · 2016-11-03T18:06:18Z

Evaluated for origin merge up to 5d22f46

openshift-bot · 2016-11-03T19:05:42Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11086/) (Base Commit: 029c0dc)

openshift-bot · 2016-11-03T19:05:43Z

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/737/) (Base Commit: 029c0dc) (Extended Tests: core(builds))

oatmealraisin · 2016-11-03T19:51:47Z

Still getting hit by custom builds

bparees · 2016-11-03T19:55:19Z

@oatmealraisin you're in the merge queue.

bparees · 2016-11-03T19:55:46Z

and @coreydaley is working on the custom build strategy failures.

oatmealraisin · 2016-11-03T20:02:00Z

@bparees Roger, just posting that I haven't abandoned the PR 👁️ 👁️

openshift-bot · 2016-11-05T00:21:39Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11167/) (Base Commit: 262bb72) (Image: devenv-rhel7_5321)

bparees reviewed Aug 9, 2016
View reviewed changes

bparees reviewed Aug 11, 2016
View reviewed changes

bparees added this to the 1.4.0 milestone Aug 11, 2016

bparees changed the title ~~oc start-build --from-repo now uses clone+checkout~~ [DO_NOT_MERGE] oc start-build --from-repo now uses clone+checkout Aug 11, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2016

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2016

bparees reviewed Sep 20, 2016

View reviewed changes

bparees changed the title ~~[DO_NOT_MERGE] oc start-build --from-repo now uses clone+checkout~~ oc start-build --from-repo now uses clone+checkout Sep 20, 2016

bparees self-assigned this Sep 21, 2016

bparees requested changes Oct 27, 2016

View reviewed changes

oc start-build --from-repo now uses clone+checkout

5d22f46

This allows us to handle submodules. However, it takes up a bit more resources. This shouldn't be a problem - even the Linux kernel only took about 3 seconds to clone to another local directory.

openshift-bot merged commit 046fad4 into openshift:master Nov 5, 2016

oc start-build --from-repo now uses clone+checkout #10316

oc start-build --from-repo now uses clone+checkout #10316

Conversation

oatmealraisin commented Aug 9, 2016

oatmealraisin commented Aug 9, 2016

Choose a reason for hiding this comment

oatmealraisin commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Aug 11, 2016

bparees commented Sep 20, 2016

oatmealraisin commented Sep 20, 2016

Choose a reason for hiding this comment

bparees commented Sep 20, 2016

bparees commented Sep 20, 2016

oatmealraisin commented Sep 21, 2016

bparees commented Sep 21, 2016

oatmealraisin commented Sep 21, 2016

oatmealraisin commented Sep 22, 2016

oatmealraisin commented Sep 26, 2016

bparees commented Sep 28, 2016

oatmealraisin commented Sep 28, 2016

oatmealraisin commented Oct 18, 2016

oatmealraisin commented Oct 26, 2016

oatmealraisin commented Oct 26, 2016

oatmealraisin commented Oct 27, 2016

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatmealraisin commented Nov 2, 2016

oatmealraisin commented Nov 3, 2016

bparees commented Nov 3, 2016

bparees commented Nov 3, 2016

oatmealraisin commented Nov 3, 2016

openshift-bot commented Nov 3, 2016

openshift-bot commented Nov 3, 2016

bparees commented Nov 3, 2016

openshift-bot commented Nov 3, 2016

openshift-bot commented Nov 3, 2016

openshift-bot commented Nov 3, 2016

oatmealraisin commented Nov 3, 2016

bparees commented Nov 3, 2016

bparees commented Nov 3, 2016

oatmealraisin commented Nov 3, 2016

openshift-bot commented Nov 5, 2016 • edited Loading

openshift-bot commented Nov 5, 2016 •

edited

Loading