Show 'Type' for PVCs in the Mounted Volumes list #1344

AyushAmbastha · 2019-03-27T08:43:39Z

Before-

After-

Note: The description I've given for PVCs in the pods.tsx file is -
'A Persistent Volume Claim is a request for a resource i.e A Persistent Volume'

openshift-ci-robot · 2019-03-27T08:43:52Z

Hi @AyushAmbastha. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

frontend/public/module/k8s/pods.ts

spadgett · 2019-03-27T11:39:44Z

frontend/public/components/utils/volume-icon.jsx

I don't think we should use the PVC resource icon here since we don't use the S for secret volumes or the CM for config map volumns.

@julim@redhat.com

Typically we use it for a specific resource instance with a link as well, so we're breaking the pattern.

Which icon do you suggest we use then?

Honestly, I'm not sure having a different icon for every kind of volume (emptydir, hostPath, secret, config map, PVC, etc.) scales. It would be difficult to find meaningful icons for all of these. We might just leave it off.

If it links to a specific secret, config map, or PVC, it would be nice to have a link to that specific object with the resource icon badge.

spadgett · 2019-05-09T17:14:16Z

Hi, I wanted to check on the status of this PR. I think the simple thing to do is to add a ResourceLink for all types that support it (PVC, secret, config map), and use a plain text value for other types.

AyushAmbastha · 2019-05-15T04:10:14Z

Hi, I wanted to check on the status of this PR. I think the simple thing to do is to add a ResourceLink for all types that support it (PVC, secret, config map), and use a plain text value for other types.

Hey @spadgett , Do you still feel that the Icon such as the 'PVC' one is not required and that Adding ResourceLink for all the ones that are supported, is enough?

AyushAmbastha · 2019-05-27T11:03:03Z

@spadgett This is how it looks like after using ResourceLink.
Also, the 'Name' column deals with the group names for volume types such as PVCs etc , but not for Secrets. So the secret name will be displayed twice. Then again, the link cant be put on the 'Name' column as it wouldn't be correct in the case of Volumes and PVCs.
What do you think we should do in this case?

spadgett · 2019-05-27T12:29:07Z

Volume name isn't always the same as the secret name, so we should display both.

The links should be like

(PVC) my-pvc
(S) my-secret

Make the actual name of the object the link text. This is consistent with other links throughout the console.

Thanks!

AyushAmbastha · 2019-05-28T08:39:57Z

@spadgett Made the changes as requested, Please review. For volume types in which neither the Resource link exists nor the className=fa-... is chosen, nothing is displayed and null is returned. Is that okay or - should be displayed?

Referring to line - https://github.com/openshift/console/pull/1344/files#diff-9f09cd1fb4d8499069d32d298cb76de7R21

frontend/public/components/utils/volume-icon.jsx

spadgett · 2019-05-29T12:00:40Z

frontend/public/module/k8s/pods.ts

s/link/kind

This comment is outstanding.

frontend/public/module/k8s/pods.ts

openshift-ci-robot · 2019-06-14T07:36:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AyushAmbastha
To complete the pull request process, please assign rhamilto
You can assign the PR to them by writing /assign @rhamilto in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

frontend/OWNERS

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

AyushAmbastha · 2019-06-14T07:43:00Z

/assign @rhamilto

frontend/public/components/utils/volume-icon.jsx

frontend/public/module/k8s/index.ts

AyushAmbastha · 2019-06-20T05:16:36Z

/assign @rhamilto

AyushAmbastha · 2019-06-20T05:17:14Z

@spadgett All changes requested have been taken care of. Please review.

spadgett

FYI, it looks like this will conflict with #1891

spadgett · 2019-07-09T12:51:07Z

frontend/public/components/volumes-table.tsx

  const kind = _.get(getVolumeType(value.volume), 'id', '');
  const loc = getVolumeLocation(value.volume);
  const name = value.name;
+  const namespace = pod.metadata.namespace;


You want to make sure you're getting the namespace from the original resource, not the pod template. I don't think it's always set here.

spadgett · 2019-07-09T12:52:10Z

frontend/public/module/k8s/pods.ts

    label: 'Projected',
    description: 'A projected volume maps several existing volume sources into the same directory.',
  },
+  persistentVolumeClaim: {


It's unclear to me why this change is needed. I'm not sure it's right since there are other more specific values for different kinds of PVCs above.

spadgett · 2019-07-09T12:52:24Z

frontend/public/module/k8s/pods.ts

This comment is outstanding.

spadgett · 2019-07-09T13:14:15Z

/assign

anmolsachan · 2019-07-15T07:19:39Z

@spadgett The contributor to this PR is no longer contributing to the project. Is this pr required or all the features are being handled in #1891 ?

anmolsachan · 2019-07-19T09:09:55Z

@zherman0 The contributor to this PR is no longer contributing to the project. Is this pr required or all the features are being handled in #1891 ?

zherman0 · 2019-07-19T15:04:57Z

@anmolsachan - PR1891 adds a kebab menu to the volume table and allows for the Remove Options. In doing so, the volume tables were refactored to use the new PF style.
That said, no additional work was done on types in the table and no work was done with any volume icons.
I think this PR is still needed if this work is considered needed, but it will need to be reworked a little once 1891 merges.

spadgett · 2019-07-19T18:21:47Z

Closing in favor of #2107

/close

openshift-ci-robot · 2019-07-19T18:21:49Z

@spadgett: Closed this PR.

Details

In response to this:

Closing in favor of #2107

/close

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.

openshift-ci-robot requested review from spadgett and zherman0 March 27, 2019 08:43

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 27, 2019

spadgett reviewed Mar 27, 2019

View reviewed changes

spadgett added this to the v4.2 milestone Apr 17, 2019

AyushAmbastha force-pushed the Pod-Volumes branch from b4819a7 to 63c373d Compare May 28, 2019 08:33

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 28, 2019

spadgett requested changes May 29, 2019

View reviewed changes

AyushAmbastha force-pushed the Pod-Volumes branch from 63c373d to 1cc382a Compare June 14, 2019 07:36

openshift-ci-robot assigned rhamilto Jun 14, 2019

spadgett requested changes Jun 19, 2019

View reviewed changes

AyushAmbastha force-pushed the Pod-Volumes branch from b9bc07b to 1c62cc9 Compare June 20, 2019 05:12

Pod volume names

e00c4f3

AyushAmbastha force-pushed the Pod-Volumes branch from 1c62cc9 to e00c4f3 Compare June 20, 2019 05:15

spadgett mentioned this pull request Jul 2, 2019

Add: Remove volume feature #1891

Merged

spadgett reviewed Jul 9, 2019

View reviewed changes

openshift-ci-robot assigned spadgett Jul 9, 2019

spadgett mentioned this pull request Jul 19, 2019

Added PVC volume type info for volume information on workloads #2105

Closed

openshift-ci-robot closed this Jul 19, 2019

Show 'Type' for PVCs in the Mounted Volumes list #1344

Show 'Type' for PVCs in the Mounted Volumes list #1344

Uh oh!

Conversation

AyushAmbastha commented Mar 27, 2019

Uh oh!

openshift-ci-robot commented Mar 27, 2019

Uh oh!

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!

spadgett commented May 9, 2019

Uh oh!

AyushAmbastha commented May 15, 2019

Uh oh!

AyushAmbastha commented May 27, 2019

Uh oh!

spadgett commented May 27, 2019

Uh oh!

AyushAmbastha commented May 28, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci-robot commented Jun 14, 2019

Uh oh!

AyushAmbastha commented Jun 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AyushAmbastha commented Jun 20, 2019

Uh oh!

AyushAmbastha commented Jun 20, 2019

Uh oh!

spadgett left a comment

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!

spadgett commented Jul 9, 2019

Uh oh!

anmolsachan commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anmolsachan commented Jul 19, 2019

Uh oh!

zherman0 commented Jul 19, 2019

Uh oh!

spadgett commented Jul 19, 2019

Uh oh!

openshift-ci-robot commented Jul 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

anmolsachan commented Jul 15, 2019 •

edited

Loading