add ExtPodKind type

[sahil143]

ODC task: https://jira.coreos.com/browse/ODC-2070

[sahil143]

/cc @christianvogt

[spadgett]

These selectors are valid for deployment configs. They just aren't type Selector.

See my change in #3017

[spadgett]

Newer resources like ReplicaSet use the new format of selector described by type Selector. Older resources use a generic name/value map.

[spadgett]

We'll eventually have a DeploymentConfigKind with

selector: MatchLabels;

and a ReplicaSetKind with

selector: Selector;

which is why I'd rather not overload what the Selector types means.

I'd like to phase out K8sResourceKind in the long run in favor of specific type declarations. For unknown types, we can have a more restrictive type like UnknownK8sResource that only allows the common properties we always expect like object metadata.

The project is large enough that I think we'd really benefit from better type checking. We've caught bugs already as we've tightened down the types we have.

[sahil143]

@spadgett Yes, I agree with your comments, these selectors are valid just not type Selector. I removed these because they are mock data and selector is not being used in the tests specifically.

[spadgett]

But they're required for deployment configs. When we add a DeploymentConfigKind type, we're going to have to add these back. It's better to leave these as-is and go with the change in #3017.

[spadgett]

This is existing code, but I dislike that this method returns something claiming to be a pod that's not. The replication controller spec and status have completely different properties than pod spec and status. At this point, I'd almost rather admit this is something completely made up and call it any. It's not a K8sResourceKind.

Can we revisit how we're handling this? It would be better to avoid creating imaginary types. I have a feeling this is going to lead to bugs down the road.

How are these ExtPodKind resources used? What scenario are we handling here and below, and how is it displayed in the UI?

[sahil143]

We use these to show status on Pod Donuts in topology view. Here Autoscaled is used to show Autoscaled to 0 on pod donut for knative deployments similarly getIdledStatus is also used to show Idled status on the pod donuts.

[spadgett]

What happens in the details pages for these resources? Does this only work in the topology view?

It seems like this logic should be in the pod donut, not here.

[christianvogt]

To be fair @sahil143 did mention originally that it was going to be a larger change to fix the type issue. I suggested that a quick solution might be to extend the type and return the pod phases back to their original. At the time I thought these types were blocking your work on improving the types. Only later did I see you were able to get around it.

If we're ok with the slower turn around time (ie possibly past 4.3 feature freeze) then your suggestion makes sense to move the logic to the donut itself where it can interpret multiple values to create the visualization instead of relying on made up data to fit the poor needs of the donut.

[sahil143]

@spadgett Yes, These are also being used in the details page for resources as there is pod donut in Overview for both admin and topology view.

[spadgett]

Sorry, I don't mean to harp on this point :) As long as this works in other contexts like the details pages and admin console workloads tab, I'm good with the short term change.

[christianvogt]

/approve

[christianvogt]

/retest

[christianvogt]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, sahil143

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:

• frontend/OWNERS [christianvogt]

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