get pipeline and pipeline run for deployment

[sahil143]

ODC Task: https://jira.coreos.com/browse/ODC-2061

This PR adds utilities in resource-utils to fetch the pipeline and pipelineRuns considering that there is a label app.openshift.io/pipeline-ref on the resource(Deployment/Deployment Config).
Fetched data can be used in topology view and Sidebar panel

[andrewballantyne]

Thanks a bunch for doing this - this will be the base for both our work.

However, a couple points need addressing below.

[andrewballantyne]

This is the concern I have with blindly using _.get. It looks great on paper... but essentially you have just created a chance for a NPE. _.get allows you to blindly walk into an object without care, but you make no attempt to validate the response before using it.

Since this appears to be the structure of this utility class, I recommend just using the 3rd param here and setting it to an empty array so the .filter a couple lines down doesn't blow up if it fails to get something back (which likely will be undefined)

[andrewballantyne]

@sahil143 I think we have had this conversation a couple times now... I'd like you to focus on your usage of _.get more closely. I worry we are going to slip and drop NPEs into our code with this kind of approach to _.get.

Either make liberal use of the 3rd param (making it the empty value of the type of what you're looking for) or avoid it at all costs, forcing the question of "what if the properties along the way are not there?".

[sahil143]

My bad, destructuring of object can be used here instead of _.get.
fixed in 3acef67

[andrewballantyne]

Is it possible for pipelineRuns not to be part of the this.resources object?

[andrewballantyne]

Boy, not a lot of types going around this utility, huh?

Do we need to update the PodRingResources interface-type?

export interface PodRingResources {
  pods: Resource;
  replicaSets: Resource;
  replicationControllers: Resource;
  deployments?: Resource;
  deploymentConfigs?: Resource;
}

[sahil143]

No, we don't need to update the PodRingResource types here because that only deals with getting the data for pods.

Is it possible for pipelineRuns not to be part of the this.resources object?

No, We added pipeline and Pipeline Run here https://github.com/openshift/console/pull/3014/files#diff-0dffb9abd12f8591b1d227efedf17dcbR113 in the resource object.

[andrewballantyne]

Right you are ... my IDE had a reference issue where it didn't track it to this import lol. @console references seem to be an issue for my IDE to track.

[invincibleJai]

as these are crd based resources, will adding optional: true make sense?

[christianvogt]

Shouldn't we be plugging into this in the same manner we've done with knative?
The console-shared package should not depend on any other packages.

[christianvogt]

This is a bad dependency.
console-shared cannot depend on dev-console.
Look at how knative plugs into the overview.

[invincibleJai]

that can be achieved from plugin.tsx in dev-console as an extension, but going forward will it be a good idea to have tekton as a separate plugin like kn.

[christianvogt]

Yup I agree. I told the same thing to @sahil143. But for now pipelines being in dev-console is fine until we get time to refactor it out.

[sahil143]

@andrewballantyne @invincibleJai @christianvogt PR is ready for review.

[andrewballantyne]

Array to single is not ideal. Typically the consumer will have an issue understanding if it's an array or not. Where is this used that you needed to change it?

[sahil143]

removed the single one in 61c377e

[sahil143]

This single instead of an array is back because pipeline in getPipelineAndPipelineRunsForResource method is an object instead of array.

[andrewballantyne]

Yeah... I see that. I am wondering if we should just just return an array for the purity of the common utility.

[christianvogt]

Easier to deal with a single variant. Leave it an array for now.

[sahil143]

fixed in 65c3b41

[andrewballantyne]

{ [key: string]: ResourceItem } expands out to be:

{
  [key: string]: {
    [key: string]: K8sResourceKind[] | K8sResourceKind;
  };
};

I doubt you were referring to a map of a map of K8sResourceKinds, where you? Because if so, we should definitely look into that.

[sahil143]

added a bit better type here 61c377e

[andrewballantyne]

Couple things here... typically you don't return undefined unless you're trying to trigger default values - which isn't what I am seeing here. You're making a call on line 56 and your type specifies it's not optional.

And the other thing is, I am pretty sure there is a logic flaw here:

Suggested change

	if (props && !props.pipelineRuns) return undefined;
	if (!props || !props.pipelineRuns) return undefined;

Otherwise on line 33 you're going to have an issue if props is not an object. If props cannot be null (or another falsey value) then you don't need to check against it in this if-condition.

[sahil143]

Thanks for noticing this. I was also a bit skeptical about the condition fixed in 61c377e

[andrewballantyne]

Same issues here with returning undefined and the conditional issue.

[sahil143]

fixed in 61c377e

[sahil143]

@karthikjeeyar @abhinandan13jan Please take a look at this PR.

[andrewballantyne]

Not that it'll help clean this up, but any reason why we are returning an object with only 1 key? Why not just the value (type in this case)?

[sahil143]

This method will execute here element https://github.com/openshift/console/pull/3014/files#diff-0dffb9abd12f8591b1d227efedf17dcbR608 and it uses the spread operator. Returning a value like this will add pipelines prop to overviewItem.

[andrewballantyne]

Something about changing this type to solve our problem feels like a mis-use of this code. All the other utilities are able to send back a map to K8sResourceKind[], I know we have two types here... perhaps we need to produce two keys... pipelines and pipelineRuns. Instead of wrapping them up in their own (PipelineOverviewItem)

[sahil143]

Agreed! After creating this utility and using through a plugin, Even I'm in favor of creating two separate keys instead of wrapping them up in their own.

[andrewballantyne]

Any reason for the sort?

[sahil143]

Yes, to have the latest run at start of the list because we want to show 3 latest runs on the side panel.

[andrewballantyne]

I wonder if this is not more suited for your usage of the code, rather than at the top level.

[andrewballantyne]

Consider the view point of possible usages:

• Would we want PipelineRuns for a table view - where the user can sort?
• Would we want PipelineRuns for a filterable view?

It's not always beneficial to apply mutations like sort on a low-level set of data.

[christianvogt]

A default sort can't hurt though.

[andrewballantyne]

Perhaps. If you're not bothered by it, I'll let it go.

[andrewballantyne]

If we are keeping sort, you should type this function. left and right are not variable names that showcase type. Ideally when reading a function, there should be limited need to trace origins to understand the nature of the function.

[sahil143]

fixed in 65c3b41

[christianvogt]

Can we avoid having these listed here?
I see the knative snuck in some values here. We'd need to clean those up as well.
Knative and pipelines shouldn't bleed into the console-shared.
Where are these being used?

[sahil143]

fixed in 65c3b41

[andrewballantyne]

@christianvogt @sahil143
So now that these are out of the types here - it's confusing reading the code.

const createTopologyNodeData = (dc: OverviewItem, cheURL?: string): TopologyDataObject => {

I read this, and go "okay, dc is of type OverviewItem, let's see what comes out of that type" - and don't find pipeline or pipelineRuns. However, if I now start up the code and throw a debugger/log in, oh look, there it is.

I get the purity of the package, but we need to consider how to address this kind of stuff across packages. TypeScript is here to help us, we definitely should be showcasing all relevant properties of an object, especially when it's typed.

[christianvogt]

Gorkem had a suggestion. Perhaps we simply find the pipeline run with the same instance label as the deployment?

[sahil143]

I doubt you mean pipeline run here. Do you mean we add the instance label to the pipeline instead of adding pipeline-ref to deployment?

[andrewballantyne]

I think he means for the other function - you're fetching all pipeline runs and tossing out everything that is not related to our pipeline that has the label.
RE: https://github.com/openshift/console/pull/3014/files#diff-bbb2e3628eb7d5b19c6b638c0968dc9dR47

[christianvogt]

I meant pipeline (corrected).
Yes instead of adding a ref on the deployment.
If a deployment and a pipeline have the same instance label, we can assume that they are related.
See https://github.com/gorkem/app-labels/blob/master/labels-annotation-for-openshift.adoc

[sahil143]

@christianvogt If a deployment doesn't have an instance label, do we fallback to metadata.name or just return null?

[sahil143]

After discussion decided to return null. fixed in 347fce3

[christianvogt]

Easier to deal with a single variant. Leave it an array for now.

[abhinandan13jan]

Although this is sample, lets not use serviceAccount field anymore

[sahil143]

fixed in 347fce3

[abhinandan13jan]

you can use _.has notation instead

[sahil143]

fixed in 347fce3

[sahil143]

/retest

[andrewballantyne]

/retest
/lgtm

[christianvogt]

/retest

[christianvogt]

/approve

[christianvogt]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, 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

[andrewballantyne]

/retest