Add workspaces to Pipeline Details Page #8238

abhinandan13jan · 2021-02-24T14:27:05Z

Addresses

In continuation to #8216
https://issues.redhat.com/browse/ODC-4444

Screenshot

PR Test

Add following YAMLs and check the details pages

apiVersion: v1
kind: Secret
metadata:
  name: secret-password
type: Opaque
data:
  password: aHVudGVyMg==
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: shared-task-storage
spec:
  resources:
    requests:
      storage: 16Mi
  volumeMode: Filesystem
  accessModes:
    - ReadWriteOnce
---
apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: fetch-secure-data
spec:
  workspaces:
  - name: super-secret-password
  - name: secure-store
  - name: filedrop
  steps:
  - name: fetch-and-write
    image: ubuntu
    script: |
      if [ "hunter2" = "$(cat $(workspaces.super-secret-password.path)/password)" ]; then
        cp $(workspaces.secure-store.path)/recipe.txt $(workspaces.filedrop.path)
      else
        echo "wrong password!"
        exit 1
      fi
---
apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: print-data
spec:
  workspaces:
  - name: storage
    readOnly: true
  params:
  - name: filename
  steps:
  - name: print-secrets
    image: ubuntu
    script: cat $(workspaces.storage.path)/$(params.filename)
---
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: fetch-and-print-recipe
spec:
  workspaces:
  - name: password-vault
  - name: recipe-store
  - name: shared-data
  tasks:
  - name: fetch-the-recipe
    taskRef:
      name: fetch-secure-data
    workspaces:
    - name: super-secret-password
      workspace: password-vault
    - name: secure-store
      workspace: recipe-store
    - name: filedrop
      workspace: shared-data
  - name: print-the-recipe
    taskRef:
      name: print-data
    # Note: this is currently required to ensure order of write / read on PVC is correct.
    runAfter:
      - fetch-the-recipe
    params:
    - name: filename
      value: recipe.txt
    workspaces:
    - name: storage
      workspace: shared-data
---
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  generateName: recipe-time-
spec:
  pipelineRef:
    name: fetch-and-print-recipe
  workspaces:
  - name: password-vault
    secret:
      secretName: secret-password
  - name: recipe-store
    configMap:
      name: sensitive-recipe-storage
      items:
      - key: brownies
        path: recipe.txt
  - name: shared-data
    persistentVolumeClaim:
      claimName: shared-task-storage

Tests

Added unit tests

Browser conformance

Chrome 88

abhinandan13jan · 2021-02-24T14:59:37Z

/retest

abhinandan13jan · 2021-02-24T15:10:44Z

@bgliwa01 @karthikjeeyar Unable to restore the commit in previous PR, please look into this one instead. regretting the inconvenience.

abhinandan13jan · 2021-02-24T15:11:48Z

@karthik I've taken care og the volumeClaimTemplate thing.

karthikjeeyar · 2021-02-24T16:18:18Z

...end/packages/pipelines-plugin/src/components/shared/workspaces/WorkspaceResourceLinkList.tsx

+                <VolumeClaimTemplateLink
+                  labels={labels}
+                  workspaceName={workspace.name}
+                  namespace={namespace}
+                />


Unique key is missing here, causing the error in console.

Suggested change

<VolumeClaimTemplateLink

labels={labels}

workspaceName={workspace.name}

namespace={namespace}

/>

<VolumeClaimTemplateLink

key={workspace.name}

labels={labels}

workspaceName={workspace.name}

namespace={namespace}

/>

karthikjeeyar · 2021-02-24T16:21:13Z

...elines-plugin/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineDetails.tsx

@@ -45,6 +46,7 @@ const PipelineDetails: React.FC<PipelineDetailsProps> = ({
            links={taskLinks}
            title={t('pipelines-plugin~Tasks')}
          />
+          <WorkspaceDefinitionList workspaces={pipeline?.spec.workspaces} />


spec is not optional but workspaces is

Suggested change

<WorkspaceDefinitionList workspaces={pipeline?.spec.workspaces} />

<WorkspaceDefinitionList workspaces={pipeline.spec?.workspaces} />

karthikjeeyar · 2021-02-24T16:28:56Z

...s/pipelines-plugin/src/components/pipelineruns/detail-page-tabs/PipelineRunCustomDetails.tsx

@@ -57,6 +58,10 @@ const PipelineRunCustomDetails: React.FC<PipelineRunCustomDetailsProps> = ({ pip
        links={renderResources}
        namespace={pipelineRun.metadata.namespace}
      />
+      <WorkspaceResourceLinkList
+        workspaces={pipelineRun?.spec.workspaces}


Suggested change

workspaces={pipelineRun?.spec.workspaces}

workspaces={pipelineRun.spec?.workspaces}

karthikjeeyar · 2021-02-24T16:31:31Z

frontend/packages/pipelines-plugin/src/components/tasks/TaskDetails.tsx

+import { TaskModel } from '../../models';
+import { TaskKind } from '../../types';
+import WorkspaceDefinitionList from '../shared/workspaces/WorkspaceDefinitionList';
+import './TaskDetails.scss';


nit: new line before scss imports

karthikjeeyar · 2021-02-24T16:36:01Z

frontend/packages/pipelines-plugin/src/utils/__tests__/pipeline-test-data.ts

@@ -290,3 +290,76 @@ export const mockRunDurationTest = [
  pipelineTestData[PipelineExampleNames.WORKSPACE_PIPELINE].pipelineRuns[DataState.SUCCESS],
  pipelineTestData[PipelineExampleNames.CLUSTER_PIPELINE].pipelineRuns[DataState.SUCCESS],
 ];
+
+export const pipelineWithWorkspace: PipelineKind[] = [


It will be good if we reuse the existing pipeline test data from pipelines-plugin/src/test-data/pipeline-data.ts. This file exclusively contains different pipeline test data including one for WORKSPACE_PIPELINE. WDYT?

using existing PL

karthikjeeyar · 2021-02-24T16:37:01Z

frontend/packages/pipelines-plugin/src/components/tasks/TaskDetails.tsx

+          <ResourceSummary resource={task} />
+        </div>
+        <div className="col-sm-6 odc-task-details__status">
+          <WorkspaceDefinitionList workspaces={task?.spec.workspaces} />


Suggested change

<WorkspaceDefinitionList workspaces={task?.spec.workspaces} />

<WorkspaceDefinitionList workspaces={task.spec?.workspaces} />

karthikjeeyar · 2021-02-24T16:37:15Z

frontend/packages/pipelines-plugin/src/components/cluster-tasks/ClusterTaskDetails.tsx

+          <ResourceSummary resource={task} />
+        </div>
+        <div className="col-sm-6 odc-cluster-task-details__status">
+          <WorkspaceDefinitionList workspaces={task?.spec.workspaces} />


Suggested change

<WorkspaceDefinitionList workspaces={task?.spec.workspaces} />

<WorkspaceDefinitionList workspaces={task.spec?.workspaces} />

karthikjeeyar · 2021-02-24T16:38:21Z

frontend/packages/pipelines-plugin/src/components/taskruns/TaskRunDetailsStatus.tsx

@@ -38,6 +39,10 @@ const TaskRunDetailsStatus = ({ taskRun }) => {
          </dd>
        </dl>
      )}
+      <WorkspaceResourceLinkList
+        workspaces={taskRun?.spec.workspaces}


Suggested change

workspaces={taskRun?.spec.workspaces}

workspaces={taskRun.spec?.workspaces}

If the TaskRun is there, I'm pretty sure Spec has to be there too... I don't think conditional checks are needed. You just have to support no workspaces (empty array or undefined) in the component.

karthikjeeyar · 2021-02-25T15:17:46Z

@bgliwa01 For Empty Directory scenario, workspace-name is shown as the text in this PR, shall we add a separate bug to add the new design changes and let this PR go in. Is it fine with you?

cc: @andrewballantyne

andrewballantyne · 2021-02-25T15:17:00Z

...end/packages/pipelines-plugin/src/components/shared/workspaces/WorkspaceResourceLinkList.tsx

+  namespace,
+}) => {
+  const { t } = useTranslation();
+  return workspaces?.length > 0 ? (


Nit: Ideally for cleaner code, you should just return early instead of making a return into a large ternary statement.

andrewballantyne · 2021-02-25T15:18:10Z

...pipelines-plugin/src/components/shared/workspaces/__tests__/WorkspaceDefinitionList.spec.tsx

+    expect(WorkspaceDefinitionListWrapper.find('dd').exists()).toBe(false);
+    expect(WorkspaceDefinitionListWrapper.find('dt').exists()).toBe(false);


Why not look for the root element? dl

andrewballantyne · 2021-02-25T15:19:09Z

...pipelines-plugin/src/components/shared/workspaces/__tests__/WorkspaceDefinitionList.spec.tsx

+
+  it('Should render workspace list', () => {
+    const { pipeline } = pipelineTestData[PipelineExampleNames.WORKSPACE_PIPELINE];
+    WorkspaceDefinitionListWrapperProps = {


Suggested change

WorkspaceDefinitionListWrapperProps = {

workspaceDefinitionListWrapperProps = {

andrewballantyne · 2021-02-25T15:22:55Z

frontend/packages/pipelines-plugin/src/components/shared/workspaces/WorkspaceDefinitionList.tsx

+  workspaces: PipelineWorkspace[];
+}
+
+const WorkspaceDefinitionList: React.FC<WorkspaceDefinitionListProps> = ({ workspaces }) => {


Do we need this component? Is this not just the same component as the WorkspaceResourceLinkList with less bells and whistles? I think they both will render a div list 🤔

Yes, the two components need different set of props to function. They also render lists based out of different types of objects.
There is also a need of certain additional mandatory props in ResourceLinkList which will not be required in DefinitionList

andrewballantyne · 2021-02-25T15:29:05Z

frontend/packages/pipelines-plugin/src/components/shared/workspaces/VolumeClaimTemplateLink.tsx

+    selector: { matchLabels: labels },
+  });
+
+  if (loaded && !loadError && pvcResource?.metadata) {


Nit: for readability, you typically want to return nulls early so that the proper render is always at the bottom. It obv doesn't impact the flow, but it helps to be consistent in a codebase this large.

thanks for the suggestion. Updated :)

andrewballantyne · 2021-02-25T15:30:11Z

frontend/packages/pipelines-plugin/src/components/shared/workspaces/VolumeClaimTemplateLink.tsx

+  const [pvcResource, loaded, loadError] = useK8sWatchResource<K8sResourceKind>({
+    kind: PersistentVolumeClaimModel.kind,
+    isList: false,
+    selector: { matchLabels: labels },
+  });


I think you also need to look at the owner field 🤔

Otherwise you're going to fetch the first PVC that matches a label and it's a coin flip if you get the one you want.

Suggested new path:

Fetch all PVCs that match the label

Look at the metadata.ownerReferences and trim down to the PLR / TR that matches

List only the remaining PVCs

Updated with this logic and also added tests

christianvogt · 2021-02-25T17:24:10Z

...pipelines-plugin/src/components/shared/workspaces/__tests__/WorkspaceDefinitionList.spec.tsx

+    expect(WorkspaceDefinitionListWrapper.find('dd').exists()).toBe(true);
+    expect(WorkspaceDefinitionListWrapper.find('dt').exists()).toBe(true);


Testing static markup here makes the test very fragile to component changes. You can omit these tag present assertions.

To test the condition that the component renders null or something, you can perform an exists check.

karthikjeeyar · 2021-03-01T16:32:57Z

frontend/packages/pipelines-plugin/src/utils/pipeline-utils.ts

+export const getMatchedPVCs: GetMatchedPVCs = (pvcResources, ownerReference) => {
+  return pvcResources.filter((pvc) => {
+    const { ownerReferences } = pvc.metadata;
+    return ownerReferences.find((reference) => reference.name === ownerReference);


metadata.ownerReferences is an optional field, If you call this function with a PVC without ownerReferences chances are this will throw a error in console, so add an empty array as a fallback value and you could also add test for the same.

Does the owner reference not include a kind/type as well? 🤔

example PLR name for instance could match another resource very easily.

updated. checking Kind as well, doing a null check and added tests

karthikjeeyar · 2021-03-01T16:41:25Z

...pelines-plugin/src/components/shared/workspaces/__tests__/WorkspaceResourceLinkList.spec.tsx

+  WorkspaceResourceLinkListProps,
+} from '../WorkspaceResourceLinkList';
+import { taskRunWithWorkspaces } from '../../../taskruns/__tests__/taskrun-test-data';
+import { SecretModel, PersistentVolumeClaimModel, ConfigMapModel } from '@console/internal/models';


nit: sort imports

karthikjeeyar · 2021-03-02T08:45:49Z

Verified locally, works fine. We need to match the Empty directory scenario once we have the design for it in a different story.
/lgtm

andrewballantyne

/lgtm cancel

I see a couple issues that will generate bugs if we leave them.

You make labels required for the fetching of the volumeClaimTemplate PVCs... which is not valid since a user could create a PLR without labels and it breaks down immediately.
The fetching on the PLRs today will cause duplicates in references as each workspace can render n-number of PVC links based on the number of volumeClaimTemplate workspaces there are. Ie, if I have two volumeClaimTemplates, you'll get 2 PVC links for each workspace:

I started a conversation with Tekton on how we could use PLRs to properly fetch them, an oversight I did not see when reviewing the designs / story -- my apologizes. They think it's a bug upstream that we don't have these values on the PLR, so this may not be fixable today -- at least not directly.

I think our best option to avoid the duplicate links is to actually go through the TRs to get the values today. We may need to fix this later if the upstream code can support PLRs knowing their PVCs that get created from volumeClaimTemplates.

~~Suggested path to fix (WDYT): see history if you want to see my message -- we don't want this as there is a bug upstream~~

andrewballantyne · 2021-03-02T14:17:14Z

frontend/packages/pipelines-plugin/src/utils/pipeline-utils.ts

+type GetMatchedPVCs = (
+  pvcResources: PersistentVolumeClaimKind[],
+  ownerReference: string,
+) => PersistentVolumeClaimKind[];
+
+export const getMatchedPVCs: GetMatchedPVCs = (pvcResources, ownerReference) => {


Typically when you're tying a function it's for when you want to make multiple functions of the same signature (like an interface)... Not that it matters a ton, but ideally gravitate towards direct types as it's easier to read.

andrewballantyne · 2021-03-02T14:17:48Z

frontend/packages/pipelines-plugin/src/utils/pipeline-utils.ts

+  return pvcResources.filter((pvc) => {
+    const { ownerReferences = [] } = pvc.metadata;
+
+    return ownerReferences.find(


Suggested change

return ownerReferences.find(

return ownerReferences.some(

Filter takes a boolean result and you don't actually need the item.

andrewballantyne · 2021-03-02T14:25:50Z

frontend/packages/pipelines-plugin/src/components/taskruns/TaskRunDetailsStatus.tsx

+      <WorkspaceResourceLinkList
+        workspaces={taskRun.spec.workspaces}
+        namespace={taskRun.metadata.namespace}
+        ownerReference={taskRun.metadata.name}


Aside from ownerReference not really speaking to the name of the prop... more like resourceName or ownerResourceName imo. But from this flow, TaskRun, you compare PipelineRun model to the owner reference.

andrewballantyne · 2021-03-02T14:29:46Z

frontend/packages/pipelines-plugin/src/components/shared/workspaces/VolumeClaimTemplateLink.tsx

+
+  if (!ownerReference) return null;
+
+  if (loaded && !loadError && pvcResources?.length > 0) {


This is a nit, but you really should return early with nulls.

Ideally you code up your component like so:

() => { if ... return null if ... return null if ... return <small rendering> // ie. an Alert return <main rendering> // ie. the main purpose of the component }

andrewballantyne · 2021-03-02T17:59:23Z

frontend/packages/pipelines-plugin/src/components/shared/workspaces/VolumeClaimTemplateLink.tsx

+  const [pvcResources, loaded, loadError] = useK8sWatchResource<PersistentVolumeClaimKind[]>({
+    kind: PersistentVolumeClaimModel.kind,
+    isList: true,
+    selector: { matchLabels: labels },
+  });


And you need to constrain this on namespace

added namespace

andrewballantyne · 2021-03-02T18:09:16Z

...pipelines-plugin/src/components/shared/workspaces/__tests__/WorkspaceDefinitionList.spec.tsx

+    workspaceDefinitionListWrapper = shallow(
+      <WorkspaceDefinitionList {...workspaceDefinitionListWrapperProps} />,
+    );
+    expect(workspaceDefinitionListWrapper.find('dl').exists()).toBe(false);


I think based on Christian's response, we want this:

Suggested change

workspaceDefinitionListWrapper = shallow(

<WorkspaceDefinitionList {...workspaceDefinitionListWrapperProps} />,

);

expect(workspaceDefinitionListWrapper.find('dl').exists()).toBe(false);

workspaceDefinitionListWrapper = shallow(

<WorkspaceDefinitionList {...workspaceDefinitionListWrapperProps} />,

);

expect(workspaceDefinitionListWrapper.isEmptyRender()).toBe(true);

Ref: https://enzymejs.github.io/enzyme/docs/api/ShallowWrapper/isEmptyRender.html

andrewballantyne · 2021-03-02T18:09:42Z

...pelines-plugin/src/components/shared/workspaces/__tests__/WorkspaceResourceLinkList.spec.tsx

+      <WorkspaceResourceLinkList {...workspaceResourceListWrapperProps} />,
+    );
+    expect(workspaceResourceListWrapper.find('dd').exists()).toBe(false);
+    expect(workspaceResourceListWrapper.find('dt').exists()).toBe(false);


You'll want to use .isEmptyRender here too... (and other places in this file)

andrewballantyne · 2021-03-02T19:43:06Z

@abhinandan13jan As requested by @bgliwa01 on slack, please drop the (workspace-name) from all the relationships links.

andrewballantyne · 2021-03-03T19:01:08Z

frontend/packages/pipelines-plugin/src/utils/__tests__/pipeline-utils.spec.ts

+  it('should return PVCs correctly matched with name and kind', () => {
+    const matchedPVCs = getMatchedPVCs(pvcWithPipelineOwnerRef, 'pipeline2', PipelineRunModel.kind);
+    expect(matchedPVCs.length).toBe(2);
+    expect(matchedPVCs[0].metadata.name).toBe(pvcWithPipelineOwnerRef[1].metadata.name);
+    expect(matchedPVCs[1].metadata.name).toBe(pvcWithPipelineOwnerRef[3].metadata.name);
+  });
+
+  it('should not match PVCs when ownerRef matches name but not kind', () => {
+    const matchedPVCs = getMatchedPVCs(pvcWithPipelineOwnerRef, 'pipeline2', PipelineRunModel.kind);
+    expect(matchedPVCs.length).toBe(2);
+    expect(matchedPVCs[0].metadata.name).toBe(pvcWithPipelineOwnerRef[1].metadata.name);
+    expect(matchedPVCs[1].metadata.name).toBe(pvcWithPipelineOwnerRef[3].metadata.name);
+  });


Same tests with different expectations? 😕

andrewballantyne · 2021-03-03T19:03:42Z

...pelines-plugin/src/components/shared/workspaces/__tests__/WorkspaceResourceLinkList.spec.tsx

+  let workspaceResourceListWrapper: ShallowWrapper<WorkspaceResourceLinkListProps>;
+  let workspaceResourceListWrapperProps: WorkspaceResourceLinkListProps;
+
+  it('Should not render dd or dt heading if no workspaces are found', () => {


Good use case for not explaining the expected output, speak to the reason for the test.

it('should not render if there are no workspaces'

andrewballantyne · 2021-03-03T19:06:22Z

...pipelines-plugin/src/components/shared/workspaces/__tests__/WorkspaceDefinitionList.spec.tsx

+  let workspaceDefinitionListWrapper: ShallowWrapper<WorkspaceDefinitionListProps>;
+  let workspaceDefinitionListWrapperProps: WorkspaceDefinitionListProps;
+
+  it('Should not render root dl if no workspaces are found', () => {


andrewballantyne · 2021-03-03T19:07:45Z

frontend/packages/pipelines-plugin/src/components/shared/workspaces/VolumeClaimTemplateLink.tsx

+          <ResourceLink
+            name={pvcResource.metadata.name}
+            key={pvcResource.metadata.name}
+            namespace={namespace}


Nit: Technically this should be pvcResource.metadata.namespace -- the logic around this makes the two match but your namespace needs to be that of the pvcResource not of your PLR/TR

andrewballantyne · 2021-03-03T19:08:55Z

...end/packages/pipelines-plugin/src/components/shared/workspaces/WorkspaceResourceLinkList.tsx

+          return <div key={workspace.name}>{workspace.name}</div>;
+        })}
+        <VolumeClaimTemplatesLink
+          namespace={namespace}
+          ownerResourceName={ownerResourceName}
+          ownerResourceKind={ownerResourceKind}
+        />


@abhinandan13jan Please test before you request a review... You are double rendering...

Or if you prefer, with my two volumeClaimTemplate example from before:

My PLR YAML, completely missed it. I've updated and tested against these now. Thanks a ton for the check

andrewballantyne · 2021-03-03T19:17:09Z

If you need access to my resources, I posted as part of the upstream request for them to add volumeClaimTemplate PVC names to the .status. See the upstream issue: tektoncd/pipeline#3800

karthikjeeyar · 2021-03-04T08:37:23Z

frontend/packages/pipelines-plugin/src/components/shared/workspaces/VolumeClaimTemplateLink.tsx

+  if (!ownerResourceName) return null;
+
+  if (!loaded || loadError || pvcResources?.length === 0) return null;


Suggested change

if (!ownerResourceName) return null;

if (!loaded || loadError || pvcResources?.length === 0) return null;

if (!ownerResourceName || !loaded || loadError || pvcResources?.length === 0) return null;

karthikjeeyar · 2021-03-04T09:18:49Z

frontend/packages/pipelines-plugin/src/components/taskruns/TaskRunDetailsStatus.tsx


 export interface TaskRunDetailsStatusProps {
  taskRun: TaskRunKind;
 }

 const TaskRunDetailsStatus = ({ taskRun }) => {
  const { t } = useTranslation();
+  const ownerReference = taskRun.metadata.ownerReferences?.[0] || taskRun.metadata.name;


taskRun.metadata.ownerReferences?.[0] This one is an object which is passed to ownerResourceName in WorkspaceResourceLinkList below. Shouldn't this be taskRun.metadata.ownerReferences?.[0].name ?

Thinking about this more, do we need this at all? TaskRun's ownerRef will be PipelineRun, but for ownerResourceKind is tied to taskrun.kind always, so it will never match 🤔 or am i missing something here?

ie:

<WorkspaceResourceLinkList workspaces={taskRun.spec.workspaces} namespace={taskRun.metadata.namespace} ownerResourceName={ownerReference} // <PipelineRun-name> (if taskrun is NOT standalone) ownerResourceKind={taskRun.kind} // TaskRun />

yes, u r correct. I misplaced the ownerReference. Updated now

<WorkspaceResourceLinkList workspaces={taskRun.spec.workspaces} namespace={taskRun.metadata.namespace} ownerResourceName={ownerReference ? ownerReference.name : taskRun.metadata.name} ownerResourceKind={ownerReference ? ownerReference.kind : taskRun.kind} />

karthikjeeyar · 2021-03-04T09:38:50Z

Workspace section is displayed without any list of workspaces for standalone Taskrun uses volumeClaimTemplate (failed due to errors). Used invalid taskRef name to reproduce this issue.

invalid_task_ref.yaml

kind: TaskRun                                                                                                                 
apiVersion: tekton.dev/v1beta1                                                                                                
metadata:                                                                                                                     
  name: test-run                                                                                                              
spec:                                                                                                                         
  taskRef:                                                                                                                    
    name: invalid-taskref-name                                                                                                               
  workspaces:                                                                                                                 
  - name: workspace                                                                                                           
    volumeClaimTemplate:                                                                                                      
      spec:                                                                                                                   
        accessModes:                                                                                                          
          - ReadWriteOnce                                                                                                     
        resources:                                                                                                            
          requests:                                                                                                           
            storage: 256M

karthikjeeyar · 2021-03-04T16:09:46Z

@abhinandan13jan You still need to fix the issue from my last comment. PTAL

karthikjeeyar · 2021-03-05T07:20:49Z

frontend/packages/pipelines-plugin/src/components/shared/workspaces/VolumeClaimTemplateLink.tsx

+          {matchedPVCs.map((pvcResource) => {
+            return (
+              <ResourceLink
+                name={pvcResource.metadata.name}
+                key={pvcResource.metadata.name}
+                namespace={pvcResource.metadata.namespace}
+                kind={PersistentVolumeClaimModel.kind}
+              />
+            );


Instead of repeating this block twice, it would be nice if you could add it to a const and contidionally render it based on the showHeader.

karthikjeeyar · 2021-03-05T07:24:49Z

Verified, everything else looks good apart from my last minor comment, I will add lgtm once that is fixed.

debsmita1 · 2021-03-05T18:12:39Z

I tried out this yaml https://github.com/tektoncd/pipeline/blob/master/examples/v1beta1/pipelineruns/workspace-from-volumeclaimtemplate.yaml

and I can see the same PVC listed twice

abhinandan13jan · 2021-03-08T09:02:40Z

I tried out this yaml https://github.com/tektoncd/pipeline/blob/master/examples/v1beta1/pipelineruns/workspace-from-volumeclaimtemplate.yaml

and I can see the same PVC listed twice

@debsmita1 @karthikjeeyar @andrewballantyne This happens if we use pipelineRun as the ownerReference for taskRun, which I did because of a review comment. I've reverted that particular change and it works fine now.

karthikjeeyar · 2021-03-08T09:57:50Z

Thanks for all the changes @abhinandan13jan
/lgtm

debsmita1 · 2021-03-08T11:21:37Z

works fine. Thanks !

openshift-ci-robot · 2021-03-09T14:50:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, andrewballantyne, karthikjeeyar

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:

~~frontend/packages/pipelines-plugin/OWNERS~~ [andrewballantyne]

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 the component/pipelines Related to pipelines-plugin label Feb 24, 2021

openshift-ci-robot requested review from andrewballantyne and christianvogt February 24, 2021 14:29

openshift-ci-robot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Feb 24, 2021

abhinandan13jan closed this Feb 24, 2021

abhinandan13jan deleted the pipeline-workspaces branch February 24, 2021 14:30

abhinandan13jan restored the pipeline-workspaces branch February 24, 2021 14:59

abhinandan13jan reopened this Feb 24, 2021

abhinandan13jan closed this Feb 24, 2021

abhinandan13jan deleted the pipeline-workspaces branch February 24, 2021 15:04

abhinandan13jan restored the pipeline-workspaces branch February 24, 2021 15:06

abhinandan13jan reopened this Feb 24, 2021

abhinandan13jan mentioned this pull request Feb 24, 2021

Add workspaces to Pipeline Details Pages #8216

Closed

karthikjeeyar reviewed Feb 24, 2021

View reviewed changes

abhinandan13jan force-pushed the pipeline-workspaces branch 2 times, most recently from 69b77b4 to 516a463 Compare February 25, 2021 12:57

andrewballantyne suggested changes Feb 25, 2021

View reviewed changes

christianvogt reviewed Feb 25, 2021

View reviewed changes

abhinandan13jan force-pushed the pipeline-workspaces branch from 516a463 to 63ea712 Compare March 1, 2021 12:24

karthikjeeyar reviewed Mar 1, 2021

View reviewed changes

abhinandan13jan force-pushed the pipeline-workspaces branch from 63ea712 to b0a76b8 Compare March 1, 2021 17:53

openshift-ci-robot assigned karthikjeeyar Mar 2, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2021

andrewballantyne suggested changes Mar 2, 2021

View reviewed changes

openshift-ci-robot assigned andrewballantyne Mar 2, 2021

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2021

andrewballantyne reviewed Mar 2, 2021

View reviewed changes

abhinandan13jan force-pushed the pipeline-workspaces branch from b0a76b8 to dc28822 Compare March 3, 2021 17:17

andrewballantyne suggested changes Mar 3, 2021

View reviewed changes

abhinandan13jan force-pushed the pipeline-workspaces branch from dc28822 to 6d6c11b Compare March 3, 2021 21:44

karthikjeeyar reviewed Mar 4, 2021

View reviewed changes

abhinandan13jan force-pushed the pipeline-workspaces branch 2 times, most recently from 7824f8d to 144625c Compare March 4, 2021 11:38

abhinandan13jan force-pushed the pipeline-workspaces branch from 144625c to 3b62136 Compare March 4, 2021 17:25

karthikjeeyar reviewed Mar 5, 2021

View reviewed changes

Add workspaces to Pipeline Details Page

c8ee149

abhinandan13jan force-pushed the pipeline-workspaces branch from 3b62136 to c8ee149 Compare March 8, 2021 08:58

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2021

andrewballantyne approved these changes Mar 9, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2021

openshift-merge-robot merged commit aaa4c6d into openshift:master Mar 9, 2021

spadgett added this to the v4.8 milestone Mar 18, 2021

	<WorkspaceDefinitionList workspaces={pipeline?.spec.workspaces} />
	<WorkspaceDefinitionList workspaces={pipeline.spec?.workspaces} />

	workspaces={pipelineRun?.spec.workspaces}
	workspaces={pipelineRun.spec?.workspaces}

	<WorkspaceDefinitionList workspaces={task?.spec.workspaces} />
	<WorkspaceDefinitionList workspaces={task.spec?.workspaces} />

	workspaces={taskRun?.spec.workspaces}
	workspaces={taskRun.spec?.workspaces}

		expect(WorkspaceDefinitionListWrapper.find('dd').exists()).toBe(false);
		expect(WorkspaceDefinitionListWrapper.find('dt').exists()).toBe(false);

	WorkspaceDefinitionListWrapperProps = {
	workspaceDefinitionListWrapperProps = {


		if (!ownerReference) return null;

		if (loaded && !loadError && pvcResources?.length > 0) {

		if (!ownerResourceName) return null;

		if (!loaded \|\| loadError \|\| pvcResources?.length === 0) return null;

Add workspaces to Pipeline Details Page #8238

Add workspaces to Pipeline Details Page #8238

Conversation

abhinandan13jan commented Feb 24, 2021 • edited Loading

Addresses

Screenshot

PR Test

Tests

Browser conformance

abhinandan13jan commented Feb 24, 2021

abhinandan13jan commented Feb 24, 2021

abhinandan13jan commented Feb 24, 2021

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

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

karthikjeeyar commented Feb 25, 2021 • 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

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

christianvogt Feb 25, 2021 • 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

Choose a reason for hiding this comment

karthikjeeyar commented Mar 2, 2021

andrewballantyne left a comment • 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

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

andrewballantyne commented Mar 2, 2021

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

andrewballantyne commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinandan13jan Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

karthikjeeyar commented Mar 4, 2021 • edited Loading

karthikjeeyar commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikjeeyar commented Mar 5, 2021

debsmita1 commented Mar 5, 2021

abhinandan13jan commented Mar 8, 2021

karthikjeeyar commented Mar 8, 2021

debsmita1 commented Mar 8, 2021

abhinandan13jan commented Feb 24, 2021 •

edited

Loading

karthikjeeyar commented Feb 25, 2021 •

edited

Loading

christianvogt Feb 25, 2021 •

edited

Loading

andrewballantyne left a comment •

edited

Loading

abhinandan13jan Mar 4, 2021 •

edited

Loading

karthikjeeyar commented Mar 4, 2021 •

edited

Loading