Basic support for HTTP URLs in oc start-build --from-file/--from-dir #11811

mmilata · 2016-11-07T17:21:00Z

Trello card: https://trello.com/c/XxcQN7V5/1070-3-allow-url-to-be-specified-in-oc-start-build-from-file

mmilata · 2016-11-09T13:57:11Z

There is only a unit test, I assume higher-level test would be nice to have - end-to-end? extended? integration? cmd? For testing oc start-build --from-file=https://... we need a server with valid certificate signed by well-known CA, how can we test that? Use github.com which we already clone from in some tests?

bparees

instead of overload --from-dir to support "archives", what do you think about introducing a "--from-archive" argument that's used to supply an archive file we expect will be extracted before being used?

then --from-file and --from-dir could both support urls
--from-dir would not support urls, which i think makes more sense because it's weird to use -from-dir with a url since it doesn't actually spider the url or otherwise treat it as a directory of content.

bparees · 2016-11-09T14:48:14Z

pkg/cmd/cli/cmd/startbuild.go

+			br := bufio.NewReaderSize(resp.Body, 4096)
+			r = br
+			if !isArchive(br) {
+				fmt.Fprintf(out, "WARNING: the provided file may not be an archive (tar, tar.gz, or zip), use --from-file\n")


is this a warning or an error? it seems weird to report it as a warning but tell them to use a different argument.

The same warning is printed when you pass regular file to --from-dir (or pipe non-archive to oc start-build --from-dir -).

maybe we just need to extend the warning (in all locations) to something like "use --as-file to prevent extraction"

mmilata · 2016-11-10T11:05:23Z

@bparees --from-dir already works with archives - if you use it with e.g. .tar.gz the file will be uploaded and then unpacked.

Agreed that it's non-intuitive. What about making --from-archive an alias for --from-dir? That way we can use the more appropriate option in documentation but keep the behavior simple.

bparees · 2016-11-10T16:56:37Z

Agreed that it's non-intuitive. What about making --from-archive an alias for --from-dir? That way we can use the more appropriate option in documentation but keep the behavior simple.

I like it except that it would mean --from-dir would still support URLs which is still going to be confusing. I think i can live w/ it as long as it's well documented what the behavior is.

bparees · 2016-11-10T17:31:05Z

There is only a unit test, I assume higher-level test would be nice to have - end-to-end? extended? integration? cmd? For testing oc start-build --from-file=https://... we need a server with valid certificate signed by well-known CA, how can we test that? Use github.com which we already clone from in some tests?

yeah doing a binary build w/ a file off github would be an option.

mmilata · 2016-11-11T16:57:09Z

@bparees updated (still no https test though)

bparees · 2016-11-11T18:45:18Z

lgtm pending end to end tests.

mmilata · 2016-11-14T14:30:24Z

Updated with tests in test/cmd/builds.sh. They test that the builds start successfully but don't wait for them to finish (no test in that file seems to do that). Waiting for the builds to finish adds about 30-40s on my development VM.

bparees · 2016-11-14T15:49:12Z

test/integration/testdata/test-buildcli.json

@@ -55,7 +55,7 @@
        "output": {
          "to": {
            "kind": "ImageStreamTag",
-            "name": "origin-ruby-sample:latest"
+            "name": "ruby-20-centos7-buildcli:latest"


The original version of the file contained two identical BuildConfigs, ruby-sample-build-validtag and ruby-sample-build-invalidtag. There's also ImageStreamTag ruby-20-centos7-buildcli which is not referenced anywhere.

In order to test binary build with a BuildConfig it has to have correct output specification, otherwise the build pod is not created and the binary input is not sent. Thus this change.

I can create separate fixture for this test if the result is less confusing.

hm. seems like this file is a mess, but the change makes sense for now.

bparees

question on why you changed the output imagestreamtag, otherwise lgtm.

bparees · 2016-11-14T18:00:45Z

lgtm for post 3.4

mmilata · 2016-11-16T11:48:25Z

The test that downloads file from github is failing:

FAILURE after 301.676s: test/cmd/builds.sh:135: executing 'oc start-build ruby-sample-build-validtag --from-file=https://raw.githubusercontent.com/openshift/ruby-hello-world/master/Dockerfile' expecting success and text 'Uploading file from "https://raw.githubusercontent.com/openshift/ruby-hello-world/master/Dockerfile" as binary input for the build': the command returned the wrong error code
There was no output from the command.
Standard error from the command:
Uploading file from "https://raw.githubusercontent.com/openshift/ruby-hello-world/master/Dockerfile" as binary input for the build ...
Error from server: Timeout: timed out waiting for build ruby-sample-build-validtag-1 to start after 5m0s

This keeps repeating in the log:

I1115 16:07:27.132630   31427 factory.go:422] About to try and schedule pod ruby-sample-build-validtag-1-build
I1115 16:07:27.132650   31427 scheduler.go:96] Attempting to schedule pod: cmd-builds/ruby-sample-build-validtag-1-build
I1115 16:07:27.132665   31427 factory.go:445] Ignoring node fake-node with Ready condition status Unknown
I1115 16:07:27.132677   31427 listers.go:170] Node fake-node matches none of the conditions
I1115 16:07:27.132683   31427 scheduler.go:100] Failed to schedule pod: cmd-builds/ruby-sample-build-validtag-1-build
I1115 16:07:27.132688   31427 factory.go:515] Unable to schedule cmd-builds ruby-sample-build-validtag-1-build: no nodes are registered to the cluster; waiting
I1115 16:07:27.132721   31427 factory.go:594] Updating pod condition for cmd-builds/ruby-sample-build-validtag-1-build to (PodScheduled==False)

The problem is likely caused by me always executing the cmd test directly, not through hack/test-cmd.sh. Looking into this.

mmilata · 2016-11-16T14:29:32Z

Yep, when running cmd tests there's no node to run pods on. Kinda makes sense for testing just the CLI functionality. I'll move this test to extended tests.

New option, --from-archive, is an alias for --from-dir. Added because passing archive to --from-dir is counter-intuitive.

mmilata · 2016-11-21T11:55:08Z

Rebased & moved the the test to test/extended/

[testextended][extended:core(builds)]

openshift-bot · 2016-11-21T12:05:30Z

Evaluated for origin testextended up to b221c23

bparees · 2016-11-21T13:43:36Z

lgtm pending passing extended test run.

openshift-bot · 2016-11-21T13:45:32Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/819/) (Base Commit: 3b2bbe5) (Extended Tests: core(builds))

bparees · 2016-11-21T13:48:31Z

[merge]

openshift-bot · 2016-11-21T13:53:25Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11600/) (Image: devenv-rhel7_5394)

openshift-bot · 2016-11-21T13:53:26Z

Evaluated for origin merge up to b221c23

openshift-bot · 2016-11-21T13:53:26Z

[Test]ing while waiting on the merge queue

openshift-bot · 2016-11-21T14:13:18Z

Evaluated for origin test up to b221c23

openshift-bot · 2016-11-21T15:41:35Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11600/) (Base Commit: 3b2bbe5)

mmilata force-pushed the oc-start-build-url branch 3 times, most recently from 8a8ff80 to 5683e12 Compare November 9, 2016 13:43

mmilata changed the title ~~[WIP] Basic support HTTP URLs in oc start-build --from-file/--from-dir~~ Basic support HTTP URLs in oc start-build --from-file/--from-dir Nov 9, 2016

mmilata mentioned this pull request Nov 9, 2016

Using URLs with start-build --from-file/--from-dir openshift/openshift-docs#3184

Merged

bparees requested changes Nov 9, 2016

View reviewed changes

mmilata force-pushed the oc-start-build-url branch from 5683e12 to 1808a14 Compare November 11, 2016 16:10

mmilata force-pushed the oc-start-build-url branch from 1808a14 to b44e05a Compare November 11, 2016 17:01

bparees added this to the 1.5.0 milestone Nov 11, 2016

bparees self-assigned this Nov 11, 2016

mmilata force-pushed the oc-start-build-url branch from b44e05a to 80609ff Compare November 14, 2016 12:08

bparees reviewed Nov 14, 2016

View reviewed changes

mmilata changed the title ~~Basic support HTTP URLs in oc start-build --from-file/--from-dir~~ Basic support for HTTP URLs in oc start-build --from-file/--from-dir Nov 14, 2016

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

Support HTTP URLs in oc start-build --from-file

b221c23

New option, --from-archive, is an alias for --from-dir. Added because passing archive to --from-dir is counter-intuitive.

mmilata force-pushed the oc-start-build-url branch from 80609ff to b221c23 Compare November 21, 2016 11:55

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

openshift-bot merged commit 1bb3a54 into openshift:master Nov 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic support for HTTP URLs in oc start-build --from-file/--from-dir #11811

Basic support for HTTP URLs in oc start-build --from-file/--from-dir #11811

mmilata commented Nov 7, 2016 •

edited

Loading

mmilata commented Nov 9, 2016

bparees left a comment

bparees Nov 9, 2016

mmilata Nov 10, 2016

bparees Nov 10, 2016

bparees Nov 10, 2016

mmilata commented Nov 10, 2016

bparees commented Nov 10, 2016

bparees commented Nov 10, 2016

mmilata commented Nov 11, 2016

bparees commented Nov 11, 2016

mmilata commented Nov 14, 2016

bparees Nov 14, 2016

mmilata Nov 14, 2016

bparees Nov 14, 2016

bparees left a comment

bparees commented Nov 14, 2016

mmilata commented Nov 16, 2016

mmilata commented Nov 16, 2016

mmilata commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

bparees commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

bparees commented Nov 21, 2016

openshift-bot commented Nov 21, 2016 •

edited

Loading

openshift-bot commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

Basic support for HTTP URLs in oc start-build --from-file/--from-dir #11811

Basic support for HTTP URLs in oc start-build --from-file/--from-dir #11811

Conversation

mmilata commented Nov 7, 2016 • edited Loading

mmilata commented Nov 9, 2016

bparees left a comment

Choose a reason for hiding this comment

bparees Nov 9, 2016

Choose a reason for hiding this comment

mmilata Nov 10, 2016

Choose a reason for hiding this comment

bparees Nov 10, 2016

Choose a reason for hiding this comment

bparees Nov 10, 2016

Choose a reason for hiding this comment

mmilata commented Nov 10, 2016

bparees commented Nov 10, 2016

bparees commented Nov 10, 2016

mmilata commented Nov 11, 2016

bparees commented Nov 11, 2016

mmilata commented Nov 14, 2016

bparees Nov 14, 2016

Choose a reason for hiding this comment

mmilata Nov 14, 2016

Choose a reason for hiding this comment

bparees Nov 14, 2016

Choose a reason for hiding this comment

bparees left a comment

Choose a reason for hiding this comment

bparees commented Nov 14, 2016

mmilata commented Nov 16, 2016

mmilata commented Nov 16, 2016

mmilata commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

bparees commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

bparees commented Nov 21, 2016

openshift-bot commented Nov 21, 2016 • edited Loading

openshift-bot commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

mmilata commented Nov 7, 2016 •

edited

Loading

openshift-bot commented Nov 21, 2016 •

edited

Loading