better warning for users when the buildconfig is of type binary #9678

bparees · 2016-07-01T20:20:12Z

also remove some dead code
also properly parse git urls so we recognize when the
buildconfig we created really is/isn't binary.

output for binary docker type build:

$ oc new-app . --dry-run
--> Found Docker image 2a332da (4 weeks old) from Docker Hub for "centos"

    * An image stream will be created as "centos:latest" that will track the source image
    * A Docker build using binary input will be created
      * The resulting image will be pushed to image stream "centos:latest"
      * Use 'start-build --from-dir=DIR' to trigger a new build
      * WARNING: a binary build was created, you must specify --from-dir/--from-file/--from-repo when starting builds
    * This image will be deployed in deployment config "centos"
    * The image does not expose any ports - if you want to load balance or send traffic to this component
      you will need to create a service with 'expose dc/centos --port=[port]' later
    * WARNING: Image "centos" runs as the 'root' user which may not be permitted by your cluster administrator

--> Creating resources with label app=centos ...
    imagestream "centos" created
    buildconfig "centos" created
    deploymentconfig "centos" created
--> Success (DRY RUN)

output for non-binary docker-type build:

$ oc new-app . --dry-run
--> Found Docker image 2a332da (4 weeks old) from Docker Hub for "centos"

    * An image stream will be created as "centos:latest" that will track the source image
    * A Docker build using source code from https://github.com/openshift/ruby-hello-world#master will be created
      * The resulting image will be pushed to image stream "ruby-hello-world:latest"
      * Every time "centos:latest" changes a new build will be triggered
    * This image will be deployed in deployment config "ruby-hello-world"
    * The image does not expose any ports - if you want to load balance or send traffic to this component
      you will need to create a service with 'expose dc/ruby-hello-world --port=[port]' later
    * WARNING: Image "centos" runs as the 'root' user which may not be permitted by your cluster administrator

--> Creating resources with label app=ruby-hello-world ...
    imagestream "centos" created
    imagestream "ruby-hello-world" created
    buildconfig "ruby-hello-world" created
    deploymentconfig "ruby-hello-world" created
--> Success (DRY RUN)

output for binary source build:

$ oc new-app . --dry-run
--> Found image 0736ebc (10 days old) in image stream "nodejs" in project "openshift" under tag "0.10" for "nodejs"

    Node.js 0.10 
    ------------ 
    Platform for building and running Node.js 0.10 applications

    Tags: builder, nodejs, nodejs010

    * The source repository appears to match: nodejs
    * A source build using binary input will be created
      * The resulting image will be pushed to image stream "nodejs:latest"
      * Use 'start-build --from-dir=DIR' to trigger a new build
      * WARNING: a binary build was created, you must specify --from-dir/--from-file/--from-repo when starting builds
    * This image will be deployed in deployment config "nodejs"
    * Port 8080/tcp will be load balanced by service "nodejs"
      * Other containers can access this service through the hostname "nodejs"

--> Creating resources with label app=nodejs ...
    imagestream "nodejs" created
    buildconfig "nodejs" created
    deploymentconfig "nodejs" created
    service "nodejs" created
--> Success (DRY RUN)

output for non-binary source build:

$ oc new-app . --dry-run
--> Found image 0736ebc (10 days old) in image stream "nodejs" in project "openshift" under tag "0.10" for "nodejs"

    Node.js 0.10 
    ------------ 
    Platform for building and running Node.js 0.10 applications

    Tags: builder, nodejs, nodejs010

    * The source repository appears to match: nodejs
    * A source build using source code from ssh://[email protected]/bparees/nodejs-ex.git#master will be created
      * The resulting image will be pushed to image stream "nodejs-ex:latest"
      * Use 'start-build to trigger a new build
    * This image will be deployed in deployment config "nodejs-ex"
    * Port 8080/tcp will be load balanced by service "nodejs-ex"
      * Other containers can access this service through the hostname "nodejs-ex"

--> Creating resources with label app=nodejs-ex ...
    imagestream "nodejs-ex" created
    buildconfig "nodejs-ex" created
    deploymentconfig "nodejs-ex" created
    service "nodejs-ex" created
--> Success (DRY RUN)

bparees · 2016-07-01T20:25:00Z

@jorgemoralespou fyi.
@gabemontero ptal

bparees · 2016-07-01T20:25:46Z

@oatmealraisin you may also find this PR of interest.

gabemontero · 2016-07-01T20:29:55Z

pkg/generate/app/cmd/describe.go

+
+		if noSource {
+			// if we have no source, the user must always provide the source from the local dir(binary build)
+			fmt.Fprintf(out, "      * Use 'start-build --from-dir=DIR' to trigger a new build\n")


we only mention --from-dir here, but then mention all 3 --from options on the next line - potentially confusion to newbies ?? .... perhaps have the above line not mention any of hte --from options, just the standard use star-build to trigger a new build, but then have the WARNING line below mention you have to pick from 1 of the 3 --from options?

yeah i didn't feel good about any choice i could make there. honestly i wanted to just write "--from-XXXX" but that would be confusing too.

i don't really want to remove it from the start-build command because i think that's a helpful sample and frankly the most common way users will invoke it.

i might just change the warning to tell them to use --from-dir and trust that advanced users will realize there are other options.

@barees or you can list some of the options separated by a pipe. I think this is standard way of showing multiple options.

@jorgemoralespou are you talking about in the warning, or in the example command?

with the options/pipes encompassed with [ ], sure .... I'm good either with @jorgemoralespou suggestion or @bparees suggestion

pushed an update, let me know what you guys think.

[ ] would be unclear imho since that usually means optional. and angle brackets usually mean "substitute this with your value". so i'm not sure what means "pick one of the 3 things here"

gabemontero · 2016-07-01T20:47:12Z

pkg/generate/app/sourcelookup.go

@@ -269,7 +269,8 @@ func (r *SourceRepository) RemoteURL() (*url.URL, bool, error) {
 		if len(ref) > 0 {
 			remote = fmt.Sprintf("%s#%s", remote, ref)
 		}
-		if r.remoteURL, err = url.Parse(remote); err != nil {
+
+		if r.remoteURL, err = git.ParseRepository(remote); err != nil {


there you go ... i had thought either @csrwng or I had made this change in new-app, but nope ... good catch

it was made in some places but not all, which led to some interesting behavior.

wonder if there were some additions after we made the change ... no matter though at this point

also remove some dead code also properly parse git urls so we recognize when the buildconfig we created really is/isn't binary.

bparees · 2016-07-01T21:26:05Z

thanks to @oatmealraisin for reminding me i lost track of the original issue i was trying to fix which is to error out when you run start-build for a binary build and don't provide a --from-XXX arg. so removing the "fix" statement from the description. but this warning is still useful and was proposed in the issue.
[merge]

openshift-bot · 2016-07-01T21:30:39Z

[Test]ing while waiting on the merge queue

bparees · 2016-07-01T23:08:57Z

docker compose importing appears to be incredibly confused by us properly parsing git urls now:

FAILURE after 0.607s: test/cmd/newapp.sh:202: executing 'oc import docker-compose -f test/testdata/app-scenarios/docker-compose/complex/docker-compose.yml --dry-run' expecting success and text 'warning: not all docker-compose fields were honored': the command returned the wrong error code; the output content test failed
There was no output from the command.
Standard error from the command:
ERROR: ssh git clone spec error: unable to parse ssh git clone specification:  /data/src/github.com/openshift/origin-bare#master
ERROR: ssh git clone spec error: unable to parse ssh git clone specification:  /data/src/github.com/openshift/origin-bare#master
can't build "nginx": cannot obtain remote URL for repository at /data/src/github.com/openshift/origin/test/testdata/app-scenarios/docker-compose/complex/nginx
can't build "web": cannot obtain remote URL for repository at /data/src/github.com/openshift/origin/test/testdata/app-scenarios/docker-compose/complex/app

will need to investigate later.

openshift-bot · 2016-07-02T13:15:23Z

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

…0fda540c69fc119

bparees · 2016-07-02T15:47:00Z

added s2i bump commit which should fix the docker-compose git url parsing problem.

openshift-bot · 2016-07-02T15:50:21Z

Evaluated for origin merge up to d283cd2

openshift-bot · 2016-07-02T15:50:22Z

Evaluated for origin test up to d283cd2

openshift-bot · 2016-07-02T17:15:23Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5740/)

bparees force-pushed the newapp_binary branch from dac6162 to b3cfc60 Compare July 1, 2016 20:21

gabemontero reviewed Jul 1, 2016
View reviewed changes

bparees force-pushed the newapp_binary branch from b3cfc60 to 73f68a0 Compare July 1, 2016 20:40

gabemontero reviewed Jul 1, 2016
View reviewed changes

bparees changed the title ~~better warn users when the buildconfig is of type binary~~ better warning for users when the buildconfig is of type binary Jul 1, 2016

better warning for users when the buildconfig is of type binary

52bbdf0

also remove some dead code also properly parse git urls so we recognize when the buildconfig we created really is/isn't binary.

bparees force-pushed the newapp_binary branch from 73f68a0 to 52bbdf0 Compare July 1, 2016 21:05

bparees mentioned this pull request Jul 2, 2016

tolerate #refs on file path git urls openshift/source-to-image#529

Merged

bparees force-pushed the newapp_binary branch from 76d0366 to 52bbdf0 Compare July 2, 2016 03:47

bump(github.com/openshift/source-to-image): ebd41a8b24bd95716b90a05fe…

d283cd2

…0fda540c69fc119

openshift-bot merged commit 61c87f3 into openshift:master Jul 2, 2016

bparees deleted the newapp_binary branch July 5, 2016 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

better warning for users when the buildconfig is of type binary #9678

better warning for users when the buildconfig is of type binary #9678

bparees commented Jul 1, 2016 •

edited

Loading

bparees commented Jul 1, 2016

bparees commented Jul 1, 2016

gabemontero Jul 1, 2016

bparees Jul 1, 2016

jorgemoralespou Jul 1, 2016

bparees Jul 1, 2016

gabemontero Jul 1, 2016

bparees Jul 1, 2016

bparees Jul 1, 2016

gabemontero Jul 1, 2016

bparees Jul 1, 2016

gabemontero Jul 1, 2016

bparees commented Jul 1, 2016

openshift-bot commented Jul 1, 2016

bparees commented Jul 1, 2016

openshift-bot commented Jul 2, 2016 •

edited

Loading

bparees commented Jul 2, 2016

openshift-bot commented Jul 2, 2016

openshift-bot commented Jul 2, 2016

openshift-bot commented Jul 2, 2016

better warning for users when the buildconfig is of type binary #9678

better warning for users when the buildconfig is of type binary #9678

Conversation

bparees commented Jul 1, 2016 • edited Loading

bparees commented Jul 1, 2016

bparees commented Jul 1, 2016

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

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

bparees commented Jul 1, 2016

openshift-bot commented Jul 1, 2016

bparees commented Jul 1, 2016

openshift-bot commented Jul 2, 2016 • edited Loading

bparees commented Jul 2, 2016

openshift-bot commented Jul 2, 2016

openshift-bot commented Jul 2, 2016

openshift-bot commented Jul 2, 2016

bparees commented Jul 1, 2016 •

edited

Loading

openshift-bot commented Jul 2, 2016 •

edited

Loading