Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add note for storage class used on PVC grid #1200

Merged
merged 1 commit into from
Feb 1, 2017
Merged

Add note for storage class used on PVC grid #1200

merged 1 commit into from
Feb 1, 2017

Conversation

zherman0
Copy link
Member

We have a request to show which provisioner was used to create a claim. I have added a muted note next to the PVC name when a storage class was assigned.

@zherman0
Copy link
Member Author

storage_view_sc2

@spadgett
Copy link
Member

I think I'd prefer it as another table column. Easier to see at a glance.

@spadgett spadgett self-assigned this Jan 31, 2017
@spadgett spadgett self-requested a review January 31, 2017 19:35
@spadgett
Copy link
Member

cc @jwforres @erinboyd

@erinboyd
Copy link

@spadgett @zherman0 I think a column might be confusing as not all PVCs will be associated with a Storage Class. And you really don't have a way to auto hide it as this view shows all PVCs.
Thoughts?

@spadgett
Copy link
Member

@erinboyd I don't feel that strongly. Will defer to you and @jwforres. We do have tables with columns that aren't always populated like routes.

openshift_web_console

@jwforres
Copy link
Member

I think the question boils down to how often we expect people to be using storage classes vs not.

@@ -55,7 +55,9 @@
</tbody>
<tbody ng-if="(pvcs | hashSize) > 0">
<tr ng-repeat="pvc in pvcs | orderObjectsByDate : true">
<td data-title="Name"><a ng-href="{{pvc | navigateResourceURL}}">{{pvc.metadata.name}}</a></td>
<td data-title="Name"><a ng-href="{{pvc | navigateResourceURL}}">{{pvc.metadata.name}}</a>
<span ng-if="pvc | annotation : 'volume.beta.kubernetes.io/storage-class'" class="text-muted"> using storage class {{pvc | annotation : 'volume.beta.kubernetes.io/storage-class'}}</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we expect the beta to eventually drop from the annotation name or storage class to ever be added as another API field, it's probably worth adding a storageClass filter. Then we can change it in one place (and fall back to looking for this annotation if needed). Something like we do for the description annotation.

https://github.com/openshift/origin-web-console/blob/master/app/scripts/filters/resources.js#L94-L100

  .filter('storageClass', function(annotationFilter) {
    return function(pvc) {
      return annotationFilter(pvc, 'volume.beta.kubernetes.io/storage-class');
    };
  })

Then here in the view

 <span ng-if="pvc | storageClass" class="text-muted"> using storage class {{pvc | storageClass}}</span>

This also makes the markup more concise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this will make things much nicer and be easier to update in the futures. Changes added.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that!

@zherman0
Copy link
Member Author

@spadgett @erinboyd @jwforres I think whether to add a column or not will come down to how much people use the storage classes. Ideally, we would like them to be quickly adopted and every new claim is associated with a storage class. But time will tell. Let's leave it as is and we can very easily tweak it in the future.

@erinboyd
Copy link

erinboyd commented Feb 1, 2017

@jwforres @spadgett @zherman0 Ideally, yes, we would hope that the majority of customers will leverage a storage class..so I guess in that case a column seems appropriate

@spadgett
Copy link
Member

spadgett commented Feb 1, 2017

Ideally, yes, we would hope that the majority of customers will leverage a storage class..so I guess in that case a column seems appropriate

I'm OK with the PR as-is if we want to merge, unless @erinboyd you'd rather see this change

@erinboyd
Copy link

erinboyd commented Feb 1, 2017

@spadgett LGTM

@spadgett
Copy link
Member

spadgett commented Feb 1, 2017

thanks @zherman0

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 5a52ce0

@openshift-bot
Copy link

openshift-bot commented Feb 1, 2017

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1005/) (Base Commit: 4eaf288)

@openshift-bot openshift-bot merged commit 936eff7 into openshift:master Feb 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants