move pkg/build to external client and types by mfojtik · Pull Request #20659 · openshift/origin

mfojtik · 2018-08-15T18:41:08Z

Moves pkg/build to external clients and external types.

Follow ups:

Move relevant consts and vars from consts.go to library-go or openshift/api
Move extended tests to external clients
Move generator to external client (so we can get rid of internal build client)
Sort out mixed internal and external printers

bparees · 2018-08-15T18:46:52Z

this isn't going into 3.11 is it?

@adambkaplan @coreydaley @nalind fyi, i don't think this will conflict w/ anything you're doing since that code should be using the external api already, if it's doing anything.

mfojtik · 2018-08-15T19:37:35Z

@bparees I don't see a reason why this should not land in 3.11 (this is not a feature, just technical debt)

mfojtik · 2018-08-15T19:38:40Z

For CLI parts:

/cc @soltysh
/cc @juanvallejo

openshift-ci-robot · 2018-08-15T19:38:42Z

@mfojtik: GitHub didn't allow me to request PR reviews from the following users: or, for, CLI, parts.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @soltysh or @juanvallejo for CLI parts

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.

mfojtik · 2018-08-15T19:39:14Z

FYI: I will split this big commit into interesting and boring changes tomorrow (and this will be ready for review)

bparees · 2018-08-15T20:04:01Z

@bparees I don't see a reason why this should not land in 3.11 (this is not a feature, just technical debt)

well we are trying to minimize the risk of 3.11 slipping, so if there's any chance this can regress something, that is a reason not to do it in 3.11.

jwforres · 2018-08-15T20:05:02Z

why are we merging any significant changesets to the 3.11 codestream? I agree with ben here, this seems like a bad idea. Especially given that we are actively trying to get branches set up ASAP for 3.11 maintenance

derekwaynecarr · 2018-08-15T20:20:14Z

i followed up off list, but i think this pr is low risk and has future benefits.

juanvallejo · 2018-08-16T19:17:41Z

pkg/oc/cli/admin/prune/imageprune/helper.go

 	"github.com/docker/distribution/registry/api/errcode"
 	"github.com/golang/glog"

+	corev1 "k8s.io/api/core/v1"


This might conflict with @soltysh 's current PR :)

juanvallejo · 2018-08-16T19:22:40Z

pkg/oc/cli/admin/prune/imageprune/prune.go

-	from := buildapihelpers.GetInputReference(strategy)
+func (p *pruner) addBuildStrategyImageReferencesToGraph(referrer *corev1.ObjectReference, strategy buildapi.BuildStrategy, predecessor gonum.Node) []error {
+	externalStrategy := buildv1.BuildStrategy{}
+	if err := legacyscheme.Scheme.Convert(&strategy, &externalStrategy, nil); err != nil {


Could we use pkg/build/apis/build/v1#Convert_build_BuildStrategy_To_v1_BuildStrategy instead here?

both legacyscheme and this are ugly, I found legacyscheme a little less ugly as it allow us to track the usages

juanvallejo · 2018-08-16T19:24:18Z

pkg/oc/cli/cancelbuild/cancelbuild.go

 		}

-		if stateMatch && !buildutil.IsTerminalPhase(internalBuildStatus.Phase) {
+		if stateMatch && !buildapi.IsInternalTerminalPhase(internalBuildStatus.Phase) {


nit: s/IsInternalTerminalPhase/IsTerminalPhaseInternal

that sounds like the intent of that check is to determine if the phase is "internal" ;-)

"is internal terminal phase" sounds like that too. So let's go w/ @juanvallejo's suggestion so it's at least consistent w/ the other helpers, unless there's a third alternative.

juanvallejo · 2018-08-16T19:25:30Z

pkg/oc/cli/set/env.go

 							continue
 						}

+						// TODO: For Maciej: the ConvertInteralPodSpecToExternal should not be needed?


I am removing the need for this in https://github.com/openshift/origin/pull/20645/files#diff-54f6ccf572703b420951c9249df04b84L522

juanvallejo · 2018-08-16T19:27:51Z

pkg/oc/lib/describe/helpers.go

+
+		triggerExternal := &buildv1.BuildTriggerPolicy{}
+
+		if err := legacyscheme.Scheme.Convert(&trigger, triggerExternal, nil); err != nil {


Convert_build_BuildTriggerPolicy_To_v1_BuildTriggerPolicy?

juanvallejo · 2018-08-16T19:30:08Z

pkg/printers/internalversion/printer.go

 }

-func printBuild(build *buildapi.Build, w io.Writer, opts kprinters.PrintOptions) error {
+func printBuild(build *buildv1.Build, w io.Writer, opts kprinters.PrintOptions) error {


hm, does it make sense to have external handlers in the internalversion package? Might make more sense to duplicate externals into a package of their own within oc tree?

agree, I don't like mixing these here as well, but I don't want to bloat this PR with that change. Will you be ok with follow up to fix this?

Yep, followup PR is fine

juanvallejo · 2018-08-16T19:34:12Z

left a few comments for cli pieces. A portion of the changes will overlap with Maciej's graph PR

…ferencemutators

…e of helpers

mfojtik · 2018-08-21T09:04:29Z

@deads2k @bparees @soltysh this should get green tests and should merge immediately after tagged. There are couple follow ups here that we can distribute perhaps:

We need to move generator to external client and types
We need to move GIT server and template instances to external client so we can delete internal build client
@juanvallejo @soltysh I copied the pkg/envresolve to pkg/envresolve/internal_version... @juanvallejo please delete it in your PR where you switching the oc set to external
External tests should move to external client (kind of required for 2)

mfojtik · 2018-08-21T09:04:49Z

/retest

mfojtik · 2018-08-21T09:36:57Z

/retest

mfojtik · 2018-08-21T10:22:27Z

/retest

mfojtik · 2018-08-21T10:45:14Z

/retest

soltysh

/lgtm

openshift-ci-robot · 2018-08-21T11:17:12Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [mfojtik]
~~pkg/OWNERS~~ [mfojtik]
~~test/common/OWNERS~~ [mfojtik,soltysh]
~~test/extended/OWNERS~~ [mfojtik]
~~test/integration/OWNERS~~ [mfojtik]

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 requested review from bparees and deads2k August 15, 2018 18:41

mfojtik force-pushed the build-01-external branch from b7f1d9e to 81ccd3e Compare August 15, 2018 18:44

openshift-ci-robot requested review from juanvallejo and soltysh August 15, 2018 19:38

mfojtik force-pushed the build-01-external branch 6 times, most recently from 9726f21 to 96c5791 Compare August 16, 2018 14:41

juanvallejo reviewed Aug 16, 2018

View reviewed changes

mfojtik force-pushed the build-01-external branch from 96c5791 to ee165b5 Compare August 17, 2018 10:10

mfojtik added 17 commits August 21, 2018 10:32

image: update imagepolicy admission to use internalversion of imagere…

879caae

…ferencemutators

controllers: introduce external build client

8a5fd7a

build: update pkg/build/client to versioned api

77553d2

fix build conversion and validation

eca1864

add build/v1 into buildscheme

691eac2

build: move some parts of api server to external and consolidate usag…

7911bf9

…e of helpers

build: move build controllers to external types

39662f7

consolidate generator helper usage

0df1bdb

build: move webhooks to external types

58d2b57

image: fix up buildconfig trigger controller

f67827f

envresolve: copy to internal version

e1a39a8

cli: move envresolve to external types

9003b92

cli: fix up cli for external build helpers

60c41a3

template: make template instance readiness use internal build helpers

5ac288c

boring: rename metrics imports

c6b703e

fix integration build errors

442079c

extended: fix helper usage and clients in build extended tests

f6c1d1c

mfojtik force-pushed the build-01-external branch from 560d071 to f6c1d1c Compare August 21, 2018 09:02

openshift-ci-robot assigned soltysh Aug 21, 2018

soltysh approved these changes Aug 21, 2018

View reviewed changes

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

openshift-merge-robot merged commit 305555a into openshift:master Aug 21, 2018

smarterclayton mentioned this pull request Sep 2, 2018

External image controller #20769

Merged

mfojtik deleted the build-01-external branch September 5, 2018 21:07


		triggerExternal := &buildv1.BuildTriggerPolicy{}

		if err := legacyscheme.Scheme.Convert(&trigger, triggerExternal, nil); err != nil {

Conversation

mfojtik commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bparees commented Aug 15, 2018

Uh oh!

mfojtik commented Aug 15, 2018

Uh oh!

mfojtik commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 15, 2018

Uh oh!

mfojtik commented Aug 15, 2018

Uh oh!

bparees commented Aug 15, 2018

Uh oh!

jwforres commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekwaynecarr commented Aug 15, 2018

Uh oh!

juanvallejo Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanvallejo Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanvallejo commented Aug 16, 2018

Uh oh!

mfojtik commented Aug 21, 2018

Uh oh!

mfojtik commented Aug 21, 2018

Uh oh!

mfojtik commented Aug 21, 2018

Uh oh!

mfojtik commented Aug 21, 2018

Uh oh!

mfojtik commented Aug 21, 2018

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Aug 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

mfojtik commented Aug 15, 2018 •

edited

Loading

mfojtik commented Aug 15, 2018 •

edited

Loading

jwforres commented Aug 15, 2018 •

edited

Loading

juanvallejo Aug 16, 2018 •

edited

Loading

juanvallejo Aug 16, 2018 •

edited

Loading