make startbuild command external #20382

juanvallejo · 2018-07-20T17:56:45Z

Switches the startbuild command to use external versions.
Switches env utils to use external versions.

~~Depends on #20399~~

juanvallejo · 2018-07-20T18:01:24Z

pkg/pod/envresolve/env.go

@@ -71,6 +72,7 @@ func getResourceFieldRef(from *kapi.EnvVarSource, c *kapi.Container) (string, er
 }

 // GenEnvVarRefValue returns the value referenced by the supplied EnvVarSource given the other supplied information
+// TODO: how to handle this? It is also used by the build controller which deals with internal versions


@mfojtik

The set/env command needs this helper to receive a corev1.EnvVarSource and a corev1.Container.
The build controller depends on a separate helper in pkg/build/controller/common which depends on the current (internal) signature of this helper

juanvallejo · 2018-07-20T18:03:10Z

pkg/oc/cli/startbuild/startbuild.go

@@ -890,7 +888,7 @@ func gitRefInfo(repo git.Repository, dir, ref string) (buildapi.GitRefInfo, erro
 }

 // WaitForBuildComplete waits for a build identified by the name to complete
-func WaitForBuildComplete(c buildclient.BuildResourceInterface, name string) error {
+func WaitForBuildComplete(c buildclientv1.BuildResourceInterface, name string) error {


the versioned / v1 build client does not actually have a BuildResourceInterface

juanvallejo · 2018-07-23T15:41:00Z

cc @soltysh

juanvallejo · 2018-07-23T18:05:32Z

pkg/oc/cli/startbuild/startbuild.go


 	// Handle environment variables
 	cmdutil.WarnAboutCommaSeparation(o.ErrOut, o.Env, "--env")
+	// TODO: not sure what to do here, duplicate this helper to return v1 EnvVars?
 	env, _, err := utilenv.ParseEnv(o.Env, o.In)


@deads2k @soltysh

Do in-place conversion using kcmdutil.AsDefaultVersionedOrOriginal for now and leave there a TODO. In a followup to this and #20400 you'll sweep those at once.

juanvallejo · 2018-07-23T18:11:29Z

pkg/oc/cli/startbuild/startbuild.go

 	}
 	o.EnvVar = env

 	// Handle Docker build arguments. In order to leverage existing logic, we
 	// first create an EnvVar array, then convert it to []docker.BuildArg
+	// TODO: also not sure what to do here, duplicate this helper to return v1 EnvVars?


cc @deads2k @soltysh

soltysh · 2018-07-27T11:15:35Z

pkg/oc/cli/startbuild/startbuild.go

@@ -17,31 +17,31 @@ import (
 	"time"

 	"github.com/golang/glog"
+	"github.com/openshift/origin/pkg/oc/util/ocscheme"


Move down to origin

soltysh · 2018-07-27T11:16:37Z

pkg/oc/cli/startbuild/startbuild.go

-	o.BuildArgs = buildArgs
+
+	// convert internal buildargs returned from the helper to external versions
+	externalBuildArgs := []corev1.EnvVar{}


Make a helper and use here and couple lines above.

soltysh · 2018-07-27T11:20:44Z

pkg/oc/cli/startbuild/startbuild.go

-		if err = json.Unmarshal(body, &buildapiv1.Build{}); err == nil {
-			if err = runtime.DecodeInto(legacyscheme.Codecs.UniversalDecoder(), body, &newBuild); err != nil {
+		newBuild := &buildv1.Build{}
+		if err = json.Unmarshal(body, newBuild); err == nil {


This looks overly complicated, why not doing:

newBuild := &buildv1.Build{} if err := json.Unmarshal(body, newBuild); err != nil { return err } if err := o.Printer.PrintObj(newBuild, o.Out); err != nil { return err }

soltysh · 2018-07-27T11:22:47Z

pkg/build/client/v1/logs.go

@@ -0,0 +1,31 @@
+package v1


Shims? From my comparison it looks like only the parameter differs, so in-place conversions to internals and use that, at least for now.

This doesn't satisfy a complexity requirement. A duplicate here is ok.

soltysh

One nit, feel free to self-lgtm when you fix it.

soltysh · 2018-07-27T15:22:34Z

pkg/oc/cli/startbuild/startbuild.go

@@ -22,26 +22,27 @@ import (
 	"github.com/openshift/source-to-image/pkg/tar"
 	s2ifs "github.com/openshift/source-to-image/pkg/util/fs"

+	"github.com/openshift/origin/pkg/oc/util/ocscheme"


Move down to origin's imports, besides that scheme should be from k8s, right?

soltysh · 2018-07-27T15:23:29Z

/approve

openshift-ci-robot · 2018-07-27T15:25:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~contrib/completions/OWNERS~~ [juanvallejo,soltysh]
~~pkg/build/OWNERS~~ [soltysh]
~~pkg/oc/OWNERS~~ [soltysh]
~~test/cmd/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

juanvallejo · 2018-07-27T21:24:04Z

/test unit

0xmichalis · 2018-07-28T21:49:20Z

/refresh

openshift-bot · 2018-07-28T23:34:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-07-29T01:34:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2018-07-29T02:52:45Z

@juanvallejo: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/cross	`6534f83`	link	`/test cross`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2018-07-29T03:35:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from mfojtik and smarterclayton July 20, 2018 17:56

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 20, 2018

juanvallejo changed the title ~~Jvallejo/make startbuild external~~ make startbuild command external Jul 20, 2018

juanvallejo commented Jul 20, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/make-startbuild-external branch 2 times, most recently from 3ccb02a to 4f8cfa0 Compare July 23, 2018 18:04

juanvallejo commented Jul 23, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/make-startbuild-external branch from 4f8cfa0 to d938a03 Compare July 23, 2018 18:11

juanvallejo commented Jul 23, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/make-startbuild-external branch 5 times, most recently from a97ffd0 to 03d0352 Compare July 26, 2018 23:04

soltysh self-assigned this Jul 27, 2018

soltysh suggested changes Jul 27, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/make-startbuild-external branch from 03d0352 to 6534f83 Compare July 27, 2018 14:06

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2018

make build webhooks and instantiatebinary clients external

3827bed

juanvallejo force-pushed the jvallejo/make-startbuild-external branch from 6534f83 to 1ee4d08 Compare July 27, 2018 14:14

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2018

soltysh approved these changes Jul 27, 2018

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2018

make startbuild cmd external

f5655a3

juanvallejo force-pushed the jvallejo/make-startbuild-external branch from 1ee4d08 to f5655a3 Compare July 27, 2018 20:46

juanvallejo added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2018

openshift-merge-robot merged commit 757e2a8 into openshift:master Jul 29, 2018

juanvallejo deleted the jvallejo/make-startbuild-external branch July 30, 2018 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make startbuild command external #20382

make startbuild command external #20382

juanvallejo commented Jul 20, 2018 •

edited

Loading

juanvallejo Jul 20, 2018

juanvallejo Jul 20, 2018

juanvallejo commented Jul 23, 2018

juanvallejo Jul 23, 2018

soltysh Jul 24, 2018

juanvallejo Jul 23, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

deads2k Jul 27, 2018

soltysh left a comment

soltysh Jul 27, 2018

soltysh commented Jul 27, 2018

openshift-ci-robot commented Jul 27, 2018

juanvallejo commented Jul 27, 2018

0xmichalis commented Jul 28, 2018

openshift-bot commented Jul 28, 2018

openshift-bot commented Jul 29, 2018

openshift-ci-robot commented Jul 29, 2018 •

edited

Loading

openshift-bot commented Jul 29, 2018

make startbuild command external #20382

make startbuild command external #20382

Conversation

juanvallejo commented Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Jul 23, 2018

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

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Jul 27, 2018

openshift-ci-robot commented Jul 27, 2018

juanvallejo commented Jul 27, 2018

0xmichalis commented Jul 28, 2018

openshift-bot commented Jul 28, 2018

openshift-bot commented Jul 29, 2018

openshift-ci-robot commented Jul 29, 2018 • edited Loading

openshift-bot commented Jul 29, 2018

juanvallejo commented Jul 20, 2018 •

edited

Loading

openshift-ci-robot commented Jul 29, 2018 •

edited

Loading