Skip to content

Add: Remove volume feature #1891

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Jul 19, 2019
Merged

Add: Remove volume feature #1891

openshift-merge-robot merged 1 commit into from
Jul 19, 2019

Conversation

@zherman0
Copy link
Member

@zherman0 zherman0 commented Jul 2, 2019 •
edited
Loading

Add a kebab with option of Remove Volume on all volumes in order to remove said volume from a resource.

This also entails a slight refactor for the VolumesTable component which currently takes a podTemplate but must now take a full resource so that I can remove the volumes from said resource.

Original:
volume-table-no-kebab

With Kebab:
volume-table-with-kebab

With Kebab Open:
volume-table-with-kebab-open

Confirmation Screen:
remove-volume-modal

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 2, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2019
@zherman0 zherman0 changed the title Remove volume feature Add: Remove volume feature Jul 2, 2019
@zherman0
Copy link
Member Author

zherman0 commented Jul 2, 2019

/assign @spadgett

@spadgett spadgett added this to the v4.2 milestone Jul 2, 2019
spadgett
spadgett requested changes Jul 2, 2019
View reviewed changes
@zherman0 zherman0 force-pushed the remove-volume branch 3 times, most recently from a1266c9 to 276bb19 Compare July 6, 2019 00:55
@zherman0
Copy link
Member Author

zherman0 commented Jul 6, 2019

Refactored the VolumesTable component to use the PF tables which also made the new kebab handle resizing properly.

jul5-kebab

Small:
jul5-small

zherman0
zherman0 commented Jul 6, 2019
View reviewed changes
@zherman0
Copy link
Member Author

zherman0 commented Jul 9, 2019

/retest

@zherman0
Copy link
Member Author

zherman0 commented Jul 9, 2019

@spadgett - Can you please review this when you have time? Thanks.

@zherman0
Copy link
Member Author

zherman0 commented Jul 9, 2019

/test e2e-aws

spadgett
spadgett requested changes Jul 10, 2019
View reviewed changes
@spadgett spadgett mentioned this pull request Jul 10, 2019
Closed
13 tasks
spadgett
spadgett requested changes Jul 12, 2019
View reviewed changes
@anmolsachan anmolsachan mentioned this pull request Jul 15, 2019
Closed
@zherman0
Copy link
Member Author

zherman0 commented Jul 15, 2019

@spadgett - Ready for review. Thanks.

@zherman0
Copy link
Member Author

zherman0 commented Jul 15, 2019

/test e2e-aws

spadgett
spadgett reviewed Jul 17, 2019
View reviewed changes
spadgett
spadgett reviewed Jul 17, 2019
View reviewed changes
@zherman0
Copy link
Member Author

zherman0 commented Jul 17, 2019

/retest

1 similar comment
@zherman0
Copy link
Member Author

zherman0 commented Jul 18, 2019

/retest

spadgett
spadgett approved these changes Jul 18, 2019
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2019
@spadgett
Copy link
Member

spadgett commented Jul 18, 2019

/retest

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 18, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spadgett, zherman0

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:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2019
@zherman0
Copy link
Member Author

zherman0 commented Jul 18, 2019

/retest

@zherman0
Copy link
Member Author

zherman0 commented Jul 18, 2019

/test e2e-aws

@zherman0
Copy link
Member Author

zherman0 commented Jul 19, 2019

/retest

@zherman0
Copy link
Member Author

zherman0 commented Jul 19, 2019

/test e2e-aws

@anmolsachan
Copy link
Contributor

anmolsachan commented Jul 19, 2019 •
edited
Loading

I tested this PR locally

Screenshot from 2019-07-19 16-39-09

@zherman0 Is displaying the type of Volume not a part of this PR ? I see that in the initial screenshots

@zherman0
Copy link
Member Author

zherman0 commented Jul 19, 2019

/test e2e-aws

anmolsachan
anmolsachan suggested changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@anmolsachan anmolsachan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@zherman0 I suppose following should be added to

console/frontend/public/module/k8s/pods.ts

Line 86 in badfbbf

},
to show the attached PVC name and type as well.

persistentVolumeClaim: {
    id: 'persistentVolumeClaim',
    label: 'Persistent Volume Claim',
    description: 'A Persistent Volume Claim is a request for Storage from a Persistent Volume',
  },

@zherman0
Copy link
Member Author

zherman0 commented Jul 19, 2019

/retest

@zherman0
Copy link
Member Author

zherman0 commented Jul 19, 2019

/test e2e-aws

@anmolsachan anmolsachan mentioned this pull request Jul 19, 2019
Closed
@spadgett spadgett mentioned this pull request Jul 19, 2019
Merged
@openshift-merge-robot openshift-merge-robot merged commit 1f3d57a into openshift:master Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spadgett spadgett spadgett approved these changes

@benjaminapetersen benjaminapetersen Awaiting requested review from benjaminapetersen

@kyoto kyoto Awaiting requested review from kyoto

+2 more reviewers

@bmignano bmignano bmignano left review comments

@anmolsachan anmolsachan anmolsachan requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@spadgett spadgett

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

v4.2

Development

Successfully merging this pull request may close these issues.

6 participants

@zherman0 @spadgett @openshift-ci-robot @anmolsachan @bmignano @openshift-merge-robot