External graph library and company by soltysh · Pull Request #20532 · openshift/origin

soltysh · 2018-08-03T14:24:27Z

No description provided.

openshift-bot · 2018-08-03T21:27:14Z

/retest

soltysh · 2018-08-16T12:31:28Z

OK, this is ready for review, although I still have some prunning unit tests to fix.

/assign @bparees @dmage
for image and build changes

/assign @juanvallejo
for general CLI stuff

/assign @deads2k
for api changes (but small 😉 )

soltysh · 2018-08-16T12:33:02Z

pkg/oc/lib/graph/genericgraph/test/prereq-image-present-notag.yaml

      dockerStrategy:
        from:
-          kind: DockerReference
+          kind: DockerImage


@bparees that was weird change, do we somehow convert DockerReference to DockerImage during conversion from external to internal, I coulnd't find anything like that. If not, how this could ever work?

I can only speculate that this object was never being passed through build api validation?

because i don't think "DockerReference" would have ever been correct.

Yeah, that what I imagined as well, that's why this is now fixed :)

@soltysh would be nice if we can call the validation in test to ensure the objects are actually valid :-) (follow up issue?)

soltysh · 2018-08-16T12:33:50Z

pkg/oc/lib/graph/genericgraph/test/runtimeobject_nodebuilder.go

+	legacy.InstallExternalLegacyAll(scheme)
+	codecs := serializer.NewCodecFactory(scheme)
+	decoder := codecs.UniversalDeserializer()
+	obj, err := runtime.Decode(decoder, data)


@deads2k for the tests that rely on scheme I've decided to create a standalone scheme installing only the APIs I cared about, wdyt?

@deads2k for the tests that rely on scheme I've decided to create a standalone scheme installing only the APIs I cared about, wdyt?

That's a good idea. You may wan to to use k8s.io/apimachinery/pkg/api/testing.SchemeForOrDie which allows you to just list the various install methods.

soltysh · 2018-08-16T12:44:57Z

pkg/oc/lib/describe/projectstatus_test.go

+	install.InstallInternalOpenShift(scheme)
+	codecs := serializer.NewCodecFactory(scheme)
+	decoder := codecs.UniversalDecoder()
 	obj, err := runtime.Decode(decoder, data)


@deads2k I had to leave this decode as it was before instead of deserializer b/c I needed defaulting to kick in.

It's fine as a stepping stone. We'll need to sort it out before we can snip oc entirely.

juanvallejo · 2018-08-16T17:24:24Z

pkg/api/imagereferencemutators/pods.go

 // This only returns pod specs for v1 compatible objects.
 func GetPodSpecV1(obj runtime.Object) (*kapiv1.PodSpec, *field.Path, error) {
 	switch r := obj.(type) {
+


Hm, since we have cli commands that depend on this, couldn't we snip this link between oc and api helpers by creating a PodSpecGetter or similar under polymorphic helpers?

Yeah, worth discussing, but definitely not in this PR ;-)

bparees · 2018-08-16T17:50:00Z

@soltysh the commit for the builds (7240c76) looks fine.. where are the image changes? (I don't really want to dig through 94 files if i can avoid it)

juanvallejo · 2018-08-16T18:35:26Z

pkg/oc/lib/graph/kubegraph/edges.go

+			if cast.ReplicaSet.Spec.Selector != nil {
+				selector = cast.ReplicaSet.Spec.Selector.MatchLabels
+			}
+			AddManagedByControllerPodEdges(g, cast, cast.ReplicaSet.Namespace, selector)


:/ we will eventually do this in a more generic way (rather than an ever-increasing switch) right?

Hehe, we'll need to figure something out, for sure.

juanvallejo · 2018-08-16T18:41:37Z

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

-		}
+		// image.DockerImageMetadata = imagev1.DockerImage{
+		// 	ID: *configName,
+		// }


Is this a TODO? Do we still need to find a way to modify the contents of imagev1.DockerImage metadata?

Yeah, that was that failing unit tests that I fixed already.

juanvallejo · 2018-08-16T18:43:24Z

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

 }

-// getClients returns a OpenShift client and Kube client.
-func getClients(f kcmdutil.Factory) (appsclient.DeploymentConfigsGetter, buildclient.BuildInterface, imageclient.ImageInterface, kclientset.Interface, error) {


glad to see old pieces of the cmdutil factory gone

juanvallejo · 2018-08-16T18:54:34Z

A few comments, otherwise cli pieces lgtm

deads2k · 2018-08-16T19:44:30Z

vendor/k8s.io/kubernetes/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr.go

 }

 func GetValueFromIntOrPercent(intOrPercent *IntOrString, total int, roundUp bool) (int, error) {
+	if intOrPercent == nil {


per my comment upstream. I think we need to avoid value compression. You can have a defaulttozeroifnil function that you wrap the call to this with, but this method needs the distinction.

soltysh · 2018-08-17T12:33:29Z

@soltysh the commit for the builds (7240c76) looks fine.. where are the image changes? (I don't really want to dig through 94 files if i can avoid it)

I was hoping you look through both graph changes and admin cli, at least the parts that are touching images.

mfojtik · 2018-08-21T09:25:33Z

pkg/oc/lib/graph/genericgraph/graphview/petset.go

@@ -1,17 +1,17 @@
 package graphview


the file itself probably deserves a rename :-)

mfojtik · 2018-08-21T09:28:07Z

pkg/oc/lib/graph/kubegraph/analysis/hpa.go

 	for _, uncastNode := range graph.NodesByKind(kubenodes.HorizontalPodAutoscalerNodeKind) {
 		node := uncastNode.(*kubenodes.HorizontalPodAutoscalerNode)

-		cpuFound := false


what happened here?

Difference in internal vs external API.

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

pkg/oc/lib/graph/kubegraph/edges.go

 			}
 			if query.Matches(labels.Set(target.Labels)) {
-				g.AddEdge(target, to, ManagedByControllerEdgeKind)
+				g.AddEdge(target, to, appsgraph.ManagedByControllerEdgeKind)


does not sound right :-) you importing this just to get a const...

I had to move that constant, otherwise I was getting import cycles.

@soltysh maybe create some common package for const, we can handle that as follow up

mfojtik · 2018-08-21T09:33:45Z

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

-	history := imageStream.Status.Tags[tag]
-	newHistory := imageapi.TagEventList{}
+	var history imagev1.NamedTagEventList
+	for _, t := range imageStream.Status.Tags {


@soltysh this pattern repeats over and over in code... I would make this a helper, something like:

func IfHasTag(imageStream, tagName, func(t Tag) error) bool, error

I'll open a followup for these helpers.

mfojtik · 2018-08-21T09:35:23Z

pkg/oc/cli/cancelbuild/cancelbuild.go

 		}

-		if stateMatch && !buildutil.IsTerminalPhase(build.Phase) {
+		if stateMatch && !buildutil.IsTerminalPhase(build.Status.Phase) {


Not sneaky, but that's how the external API differs from internal.

soltysh · 2018-08-21T10:00:05Z

Switched the build commit to use internal helpers.

soltysh · 2018-08-21T11:15:02Z

/retest

mfojtik · 2018-08-21T11:17:56Z

/lgtm

let the race begin!

…m appsapi entirely

openshift-ci-robot · 2018-08-21T13:31:35Z

New changes are detected. LGTM label has been removed.

openshift-ci-robot · 2018-08-21T13:31:41Z

[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/api/OWNERS~~ [mfojtik]
~~pkg/apps/OWNERS~~ [mfojtik,soltysh]
~~pkg/image/OWNERS~~ [mfojtik,soltysh]
~~pkg/oc/OWNERS~~ [mfojtik,soltysh]
~~test/extended/OWNERS~~ [mfojtik]
~~vendor/k8s.io/kubernetes/pkg/controller/deployment/OWNERS~~ [mfojtik]
~~vendor/k8s.io/kubernetes/staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [mfojtik]

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

soltysh · 2018-08-21T13:32:43Z

Rebased - tagging back.

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 3, 2018

openshift-ci-robot requested review from juanvallejo and smarterclayton August 3, 2018 14:24

soltysh force-pushed the external_images branch 2 times, most recently from ae911b5 to ea2ce08 Compare August 14, 2018 13:50

soltysh mentioned this pull request Aug 16, 2018

Add missing image API types openshift/api#86

Merged

soltysh force-pushed the external_images branch from ea2ce08 to c56f5c0 Compare August 16, 2018 12:29

soltysh changed the title ~~WIP: External graph library and company~~ External graph library and company 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

openshift-ci-robot assigned bparees, deads2k, dmage and juanvallejo Aug 16, 2018

soltysh commented Aug 16, 2018

View reviewed changes

juanvallejo reviewed Aug 16, 2018

View reviewed changes

juanvallejo mentioned this pull request Aug 16, 2018

move pkg/build to external client and types #20659

Merged

4 tasks

deads2k reviewed Aug 16, 2018

View reviewed changes

juanvallejo mentioned this pull request Aug 16, 2018

Externalize newapp / newbuild #20645

Merged

soltysh force-pushed the external_images branch from c56f5c0 to 8536d76 Compare August 17, 2018 13:10

mfojtik reviewed Aug 21, 2018

View reviewed changes

soltysh force-pushed the external_images branch from 005272b to 33efe28 Compare August 21, 2018 09:59

soltysh force-pushed the external_images branch from 33efe28 to b763576 Compare August 21, 2018 11:00

openshift-ci-robot assigned mfojtik Aug 21, 2018

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 21, 2018

soltysh added 9 commits August 21, 2018 14:47

UPSTREAM: 67493: Tolerate nil input in GetValueFromIntOrPercent

42ddf9f

Move apps helpers to graph helpers

21de3b7

Drop DeploymentConfigAnnotation and DeploymentCancelledAnnotation fro…

8bcd18a

…m appsapi entirely

Add missing versions to GetPodSpec

267538d

Change manual conversions for image's meta

0fe8af2

Externalize graph library

ca4d279

Externalize admin commands depending on graph library

28e770e

Fix DockerImageMetadata reading when dealing with versioned client

12ac77b

Update import restrictions

d14912b

soltysh force-pushed the external_images branch from b763576 to d14912b Compare August 21, 2018 13:31

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

soltysh added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2018

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

soltysh deleted the external_images branch August 22, 2018 12:38

Conversation

soltysh commented Aug 3, 2018

Uh oh!

openshift-bot commented Aug 3, 2018

Uh oh!

soltysh commented Aug 16, 2018

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!

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!

bparees commented Aug 16, 2018

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!

juanvallejo commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh commented Aug 17, 2018

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!

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!

soltysh commented Aug 21, 2018

Uh oh!

soltysh commented Aug 21, 2018

Uh oh!

mfojtik commented Aug 21, 2018

Uh oh!

openshift-ci-robot commented Aug 21, 2018

Uh oh!

openshift-ci-robot commented Aug 21, 2018

Uh oh!

soltysh commented Aug 21, 2018

Uh oh!

Reviewers

juanvallejo Aug 16, 2018 •

edited

Loading