Externalize newapp / newbuild #20645

juanvallejo · 2018-08-14T23:22:17Z

~~Work in progress, does not build yet.~~ It builds! Creates a few TODOs, externalizes "oc set env" as a side-effect :)
Decouples more apihelpers.

~~Depends on #20532~~

cc @soltysh @deads2k

soltysh

I left you some comments and suggestions already :-)

soltysh · 2018-08-15T08:31:30Z

pkg/build/buildapihelpers/helpers.go

+// TODO(juanvallejo): change internal helper to "GetInputReferenceInternal" once https://github.com/openshift/origin/pull/20532 merges
+// GetInputReferenceV1 returns the From ObjectReference associated with the
+// BuildStrategy.
+func GetInputReferenceV1(strategy buildv1.BuildStrategy) *corev1.ObjectReference {


Not like this, see how it's done in https://github.com/openshift/origin/pull/20532/files#diff-5cb0d8a77f515d00d8e768a169ded24c it'll be easier for both of us to have those in sync. Besides never create *V1 versions, rather call the old one *Internal and let the external version re-use the name.

You can probably just pick the commit that contains all those fixes from that PR.

Will do - wanted to do it this way at first, but did not want to overlap changes in that PR. Seeing as how your PR will merge first anyway, I'll go ahead and apply your changes here

soltysh · 2018-08-15T08:33:43Z

pkg/oc/cli/set/env.go

@@ -30,9 +28,8 @@ import (

 	buildv1 "github.com/openshift/api/build/v1"
 	cmdutil "github.com/openshift/origin/pkg/cmd/util"
-	"github.com/openshift/origin/pkg/oc/util/clientcmd"
+	envresolve "github.com/openshift/origin/pkg/oc/lib/env"
 	utilenv "github.com/openshift/origin/pkg/oc/util/env"


Wait, we have two utils for working with env, we should merge them together.

The utils from the envresolve package depend on the ResourceStore in that same package. They are also used by the build controller. Maybe we could duplicate them and have them live in the oc/lib tree? Misunderstood, initially thought you were referencing the controller helper and the newly created oc/cli envresolve file.

Will go ahead and merge these two cli helpers

soltysh · 2018-08-15T08:36:25Z

pkg/oc/cli/set/env.go

 			resolutionErrorsEncountered := false
-			containers, _ := selectContainers(spec.Containers, o.ContainerSelector)
+			containers, _ := selectContainersV1(spec.Containers, o.ContainerSelector)


selectContainers should work with external versions, selectContainersInternal with internals.

soltysh · 2018-08-15T08:36:50Z

pkg/oc/cli/set/helper.go

@@ -26,6 +27,20 @@ func selectContainers(containers []kapi.Container, spec string) ([]*kapi.Contain
 	return out, skipped
 }

+// TODO(juanvallejo): remove internal helper once all callers are switched to externals
+func selectContainersV1(containers []corev1.Container, spec string) ([]*corev1.Container, []*corev1.Container) {


See my other comment about naming this.

soltysh · 2018-08-15T08:37:30Z

pkg/oc/lib/env/envresolve.go

@@ -0,0 +1,168 @@
+package env


See my other comment about this file and package. One of them needs to die, having two will cause confusion and may lead to code duplication.

soltysh · 2018-08-15T08:40:10Z

pkg/oc/lib/newapp/app/templatelookup.go

@@ -13,14 +13,14 @@ import (
 	kapi "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource"

+	templatev1 "github.com/openshift/api/template/v1"
+	templatev1client "github.com/openshift/client-go/template/clientset/versioned/typed/template/v1"
 	"github.com/openshift/origin/pkg/oc/util/ocscheme"


While you're at it, figure out what this is used for and if this can be replaced with kubectl's scheme.

soltysh · 2018-08-15T08:42:31Z

pkg/oc/lib/newapp/app/imagestreamlookup.go

+							}
+
+							// TODO(juanvallejo): we're no longer using an api helper here - but are duplicating logic
+							// from internal image api helper to make it suitable for the v1 api. Is this okay?


Image controller will eventually need the external versions. Make the helpers in image package in that case. At some point in time when we'll decide to split oc out we'll have to figure something out.

juanvallejo · 2018-08-21T14:30:17Z

/retest

soltysh · 2018-08-22T14:32:40Z

pkg/oc/cli/admin/migrate/images/imagerefs.go

@@ -24,7 +24,7 @@ import (
 	imageapi "github.com/openshift/origin/pkg/image/apis/image"
 	imageclientinternal "github.com/openshift/origin/pkg/image/generated/internalclientset"
 	imagetypedclient "github.com/openshift/origin/pkg/image/generated/internalclientset/typed/image/internalversion"
-	"github.com/openshift/origin/pkg/oc/util/clientcmd"
+	"k8s.io/kubernetes/pkg/api/legacyscheme"


Move the import up.

soltysh · 2018-08-22T14:33:43Z

pkg/oc/cli/admin/registry/registry.go

+	for _, e := range env.List() {
+		internal := &kapi.EnvVar{}
+		err := kapiv1.Convert_v1_EnvVar_To_core_EnvVar(&e, internal, nil)
+		if err != nil {


Log the error.

Thanks, done

soltysh · 2018-08-22T14:37:22Z

pkg/oc/lib/newapp/app/imagestreamlookup_test.go


+	"encoding/json"


Move this import up to all of golang imports.

soltysh · 2018-08-22T14:40:11Z

pkg/oc/lib/newapp/cmd/newapp.go

+				if err != nil {
+					return nil, err
+				}
+				objects = append(objects, runtime.RawExtension{


This looks very wrong.

Yeah, I am a bit hesitant about doing this as well. Maybe it is worth leaving QueryResult.List as an internal list, rather than corev1.List

soltysh · 2018-08-22T14:43:56Z

pkg/oc/lib/newapp/cmd/newapp.go

@@ -789,6 +814,24 @@ func (c *AppConfig) validate() (app.Environment, app.Environment, app.Environmen
 	return env, buildEnv, params, nil
 }

+func templateObjectsToAppObjects(objs []runtime.RawExtension) (app.Objects, error) {
+	converted := []runtime.Object{}


This should not be needed, as well.

hm, I needed a way to take all of the raw.Extension objects I receive from building the (now) external templates, and convert them into []runtime.Object

soltysh · 2018-08-22T14:45:39Z

pkg/oc/lib/newapp/cmd/template.go

 	templateclientv1 "github.com/openshift/origin/pkg/template/client/v1"
 )

-// TODO: remove this helper once all consumers are switched to external versions


juanvallejo · 2018-09-07T19:28:44Z

/test end_to_end

juanvallejo · 2018-09-07T19:49:55Z

/retest

juanvallejo · 2018-09-08T03:00:52Z

/test cmd

juanvallejo · 2018-09-08T22:01:20Z

/retest

juanvallejo · 2018-09-09T02:05:45Z

/test gcp

soltysh · 2018-09-10T12:14:50Z

pkg/oc/cli/newapp/newapp.go

@@ -344,7 +344,18 @@ func (o *AppOptions) RunNewApp() error {
 		}

 		if o.Action.ShouldPrint() {
-			return o.Printer.PrintObj(result.List, o.Out)
+			// TODO(juanvallejo): this needs to be fixed by updating QueryResult.List to be of type corev1.List


Discussed with @juanvallejo, this will be fixed in a followup.

Tracked in #20919

soltysh

/lgtm
/approve

soltysh · 2018-09-10T12:16:00Z

pkg/oc/cli/newapp/newapp.go

-	config.KubeClient = kclient
+
+	config.KubeClient, err = kubernetes.NewForConfig(clientConfig)
+


I don't know why you're such a fan of empty lines 😜, you put quite a lot of them in your PRs.

Makes things less claustrophobic? :)

soltysh · 2018-09-10T12:17:05Z

pkg/oc/lib/newapp/app/app.go

 				From: &objRef,
 			},
 		})
-		source.Images = []buildapi.ImageSource{imgSrc}
+		source.Images = []buildv1.ImageSource{imgSrc}


I'm guessing this was one of your problems. I fixed a similar problem in the generator as well.

soltysh · 2018-09-10T12:27:03Z

Applying the approve label manually, since all the changes are CLI-triggered from new-app/new-build commands switch to externals.

openshift-ci-robot · 2018-09-10T12:28:12Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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]
hack/OWNERS
pkg/OWNERS
test/integration/OWNERS
test/testdata/OWNERS

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

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 14, 2018

openshift-ci-robot requested review from jim-minter and mfojtik August 14, 2018 23:22

soltysh suggested changes Aug 15, 2018

View reviewed changes

soltysh self-assigned this Aug 15, 2018

juanvallejo force-pushed the jvallejo/update-newapp-external branch 2 times, most recently from 3162dfb to b423bc8 Compare August 16, 2018 22:49

juanvallejo changed the title ~~[WIP] Externalize newapp / newbuild~~ Externalize newapp / newbuild Aug 16, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2018

juanvallejo force-pushed the jvallejo/update-newapp-external branch 3 times, most recently from ee8151c to a47edb9 Compare August 17, 2018 17:04

juanvallejo mentioned this pull request Aug 20, 2018

Fix imagestream bugs #20666

Merged

juanvallejo force-pushed the jvallejo/update-newapp-external branch 3 times, most recently from f042e27 to 8231223 Compare August 21, 2018 14:17

juanvallejo force-pushed the jvallejo/update-newapp-external branch 4 times, most recently from 9d22c12 to c00eee8 Compare August 21, 2018 23:11

soltysh suggested changes Aug 22, 2018

View reviewed changes

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

juanvallejo force-pushed the jvallejo/update-newapp-external branch from c00eee8 to eacc705 Compare August 31, 2018 03:15

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

juanvallejo force-pushed the jvallejo/update-newapp-external branch from 81b8f77 to 0da4237 Compare August 31, 2018 21:02

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

juanvallejo force-pushed the jvallejo/update-newapp-external branch 2 times, most recently from bd6592f to b951eba Compare August 31, 2018 21:23

juanvallejo force-pushed the jvallejo/update-newapp-external branch from acb2b41 to f119626 Compare September 7, 2018 19:46

juanvallejo force-pushed the jvallejo/update-newapp-external branch from f119626 to 3e4322d Compare September 7, 2018 19:52

juanvallejo force-pushed the jvallejo/update-newapp-external branch 3 times, most recently from 87202a1 to 3a5c87b Compare September 8, 2018 21:49

juanvallejo added 9 commits September 8, 2018 18:18

externalize newapp command

0cc42c8

externalize newbuild command

7a39888

externalize set commands

142c7d0

update admin commands, deal with external envs

e7a37df

externalize startbuild command

8f4d19b

remove internal conversion debug command

b68c5d6

remove internal conversion appjson command

f5016a3

update import restrictions

5ce1caa

update completions

40f620e

juanvallejo force-pushed the jvallejo/update-newapp-external branch from 3a5c87b to 40f620e Compare September 8, 2018 22:18

soltysh reviewed Sep 10, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2018

soltysh approved these changes Sep 10, 2018

View reviewed changes

soltysh added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2018

openshift-merge-robot merged commit 5ab1a78 into openshift:master Sep 10, 2018

juanvallejo deleted the jvallejo/update-newapp-external branch September 10, 2018 13:52

juanvallejo mentioned this pull request Sep 10, 2018

Improve printing in newapp #20919

Closed

gabemontero mentioned this pull request Apr 25, 2019

Newapp crd 311 #22654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Externalize newapp / newbuild #20645

Externalize newapp / newbuild #20645

juanvallejo commented Aug 14, 2018 •

edited

Loading

soltysh left a comment

soltysh Aug 15, 2018

soltysh Aug 15, 2018

juanvallejo Aug 16, 2018

soltysh Aug 15, 2018

juanvallejo Aug 17, 2018 •

edited

Loading

soltysh Aug 15, 2018

soltysh Aug 15, 2018

soltysh Aug 15, 2018

soltysh Aug 15, 2018

soltysh Aug 15, 2018

juanvallejo commented Aug 21, 2018

soltysh Aug 22, 2018

soltysh Aug 22, 2018

juanvallejo Sep 4, 2018

soltysh Aug 22, 2018

soltysh Aug 22, 2018

juanvallejo Aug 31, 2018

soltysh Aug 22, 2018

juanvallejo Aug 31, 2018

soltysh Aug 22, 2018

juanvallejo commented Sep 7, 2018

juanvallejo commented Sep 7, 2018

juanvallejo commented Sep 8, 2018

juanvallejo commented Sep 8, 2018

juanvallejo commented Sep 9, 2018

soltysh Sep 10, 2018

juanvallejo Sep 10, 2018

soltysh left a comment

soltysh Sep 10, 2018

juanvallejo Sep 10, 2018

soltysh Sep 10, 2018

soltysh commented Sep 10, 2018

openshift-ci-robot commented Sep 10, 2018

		config.KubeClient = kclient

		config.KubeClient, err = kubernetes.NewForConfig(clientConfig)

Externalize newapp / newbuild #20645

Externalize newapp / newbuild #20645

Conversation

juanvallejo commented Aug 14, 2018 • edited Loading

soltysh 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

juanvallejo Aug 17, 2018 • edited Loading

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

juanvallejo commented Aug 21, 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

Choose a reason for hiding this comment

juanvallejo commented Sep 7, 2018

juanvallejo commented Sep 7, 2018

juanvallejo commented Sep 8, 2018

juanvallejo commented Sep 8, 2018

juanvallejo commented Sep 9, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Sep 10, 2018

openshift-ci-robot commented Sep 10, 2018

juanvallejo commented Aug 14, 2018 •

edited

Loading

juanvallejo Aug 17, 2018 •

edited

Loading