Skip to content

Conversation

@bgaydosrh
Copy link

@bgaydosrh bgaydosrh commented Jul 14, 2020

This PR replaces #22638

Label enterprise-4.5 enterprise-4.6

Tagging @adellape for peer review.

All comments in previous PR have been acted upon and content is ready for final review.

See http://file.bos.redhat.com/bgaydos/072020/virt/virtual_machines/virtual_disks/virt-features-for-storage.html for test build.

Thanks,
Bob

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 14, 2020
@openshift-docs-preview-bot

The preview will be available shortly at:

@apinnick
Copy link
Contributor

apinnick commented Jul 15, 2020

@nellyc Please review the "VM import" column.

Update: Nelly has approved the VM import column.

@nellyc
Copy link

nellyc commented Jul 15, 2020

I am wondering if 'supported' is a good word to use in the table,
as QE are not covering all storage types.
@dankenigsberg WDYT?

@dankenigsberg
Copy link
Contributor

@nellyc I share your sentiment. "Supported" may misleading the reader to understand that Red Hat would support any CSI driver, while it is the CSI vendor that should do it.

I think that "acceptable" or simply the tick mark would be better.

@bgaydosrh
Copy link
Author

bgaydosrh commented Jul 16, 2020

@nellyc I share your sentiment. "Supported" may misleading the reader to understand that Red Hat would support any CSI driver, while it is the CSI vendor that should do it.

I think that "acceptable" or simply the tick mark would be better.

@dankenigsberg and @nellyc would you be fine with either YES for Supported and NO for Not Supported? We try to stay away from graphics in tables. The other alternative I can think of is using "X" and "O" and then defining that in the table key.

I pinged @aglitke and he is fine with YES and NO, he likes it.

@nellyc
Copy link

nellyc commented Jul 16, 2020

Im fine with Yes/No

Copy link
Contributor

@fabiand fabiand left a comment

Choose a reason for hiding this comment

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

Some things still need calrification

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2020
@bgaydosrh
Copy link
Author

Some things still need calrification

@fabiand Please review the latest comments and suggest clarifications if still needed. I am hoping to get this into peer review as soon as possible since this is our last week before release. Thanks, Bob

@bgaydosrh
Copy link
Author

Im fine with Yes/No

Changed "Supported" to "Yes" and "Not Supported" to "No".

@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 20, 2020
@bgaydosrh bgaydosrh force-pushed the CNV-3924_2 branch 2 times, most recently from f3aadd2 to 5f4579d Compare July 20, 2020 17:02
@aglitke
Copy link

aglitke commented Jul 20, 2020

lgtm

@nellyc
Copy link

nellyc commented Jul 22, 2020

I think my only problem is with the vm import now
there is a difference between vmware & rhv imports
vmware import works with rwx, but rhv import doesn't
not sure how we should display it

@bgaydosrh
Copy link
Author

I think my only problem is with the vm import now
there is a difference between vmware & rhv imports
vmware import works with rwx, but rhv import doesn't
not sure how we should display it

@nellyc We might be able to fix this with a footnote to that effect. @alitke what do you think?

@bgaydosrh bgaydosrh force-pushed the CNV-3924_2 branch 2 times, most recently from c648e7e to 2e84f5c Compare July 23, 2020 14:50
@adellape adellape self-requested a review July 23, 2020 21:23
Copy link
Contributor

@adellape adellape left a comment

Choose a reason for hiding this comment

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

Requesting footnote style changes in the table, but otherwise LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@adellape adellape left a comment

Choose a reason for hiding this comment

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

👍

@adellape adellape added branch/enterprise-4.5 branch/enterprise-4.6 peer-review-done Signifies that the peer review team has reviewed this PR labels Jul 24, 2020
@adellape adellape added this to the Next Release milestone Jul 24, 2020
@adellape adellape merged commit 0a19b68 into openshift:master Jul 24, 2020
@adellape
Copy link
Contributor

/cherrypick enterprise-4.5

@adellape
Copy link
Contributor

/cherrypick enterprise-4.6

@openshift-cherrypick-robot

@adellape: new pull request created: #24174

Details

In response to this:

/cherrypick enterprise-4.5

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-cherrypick-robot

@adellape: new pull request created: #24175

Details

In response to this:

/cherrypick enterprise-4.6

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.5 branch/enterprise-4.6 peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.