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

BZ1980340: [DOC-OCPRHV] Add resize to ovirt CSI driver #34475

Merged
merged 1 commit into from Aug 5, 2021

Conversation

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 13, 2021
@netlify
Copy link

netlify bot commented Jul 13, 2021

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: 6e90398

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/610ba8f5edde5a0008b2ed5d

😎 Browse the preview: https://deploy-preview-34475--osdocs.netlify.app

@openshift-ci openshift-ci bot 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 Jul 13, 2021
@stoobie
Copy link
Contributor Author

stoobie commented Jul 13, 2021

@Gal-Zaidman Please review.

@Gal-Zaidman
Copy link

  • Not sure where it is in the code, but the title on[1] needs to be renamed to "Red Hat Virtualization (RHV) object definition" to align to the rest of the platforms.
  • The storage is missing one more space

[1] https://deploy-preview-34475--osdocs.netlify.app/openshift-enterprise/latest/post_installation_configuration/storage-configuration.html#ovirt-csi-driver-storage-class_post-install-storage-configuration

@stoobie
Copy link
Contributor Author

stoobie commented Jul 14, 2021

* Not sure where it is in the code, but the title on[1] needs to be renamed to "Red Hat Virtualization (RHV) object definition" to align to the rest of the platforms.

@Gal-Zaidman Is the rest of the topic OK as-is?

@stoobie stoobie force-pushed the 1980340 branch 2 times, most recently from f56f6b5 to 3bba269 Compare July 15, 2021 09:48
@Gal-Zaidman
Copy link

Things look OK to me I think you fixed all my comments

@vikram-redhat vikram-redhat changed the title 1980340 - [DOC-OCPRHV] Add resize to ovirt CSI driver BZ1980340 - [DOC-OCPRHV] Add resize to ovirt CSI driver Jul 15, 2021
@vikram-redhat vikram-redhat changed the title BZ1980340 - [DOC-OCPRHV] Add resize to ovirt CSI driver BZ1980340: [DOC-OCPRHV] Add resize to ovirt CSI driver Jul 15, 2021
@stoobie
Copy link
Contributor Author

stoobie commented Jul 21, 2021

@Gal-Zaidman : @mgold1234 thinks it would be helpful to add explanations of these two items:

reclaimPolicy: Delete
volumeBindingMode: Immediate

What do you think?

@stoobie stoobie force-pushed the 1980340 branch 5 times, most recently from 972d0f0 to 83dbe6e Compare July 26, 2021 09:35
@bobfuru bobfuru self-requested a review July 26, 2021 14:09
@bobfuru bobfuru added this to the Future Release milestone Jul 26, 2021
Copy link
Contributor

@bobfuru bobfuru left a comment

Choose a reason for hiding this comment

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

A couple of minor requests and otherwise this LGTM! Also, not a part of this PR but I noticed a typo in the module "Creating a persistent volume on RHV" (persistent-storage-rhv.adoc), the first line of the procedure: "If you are using the we console...." Would you mind fixing this as well? 🙇


{product-title} creates a default object of type `StorageClass` named `ovirt-csi-sc` which is used for creating dynamically provisioned persistent volumes.

To create additional storage classes for different configurations, create and save a file with the `StorageClass` object described by the following sample YAML:

.ovirt-storageclass.yaml
[source,yaml]
----
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
name: <storage-class-name> <1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please make this storage_class_name (use underscores) to be in line with our convention? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks for pointing this out.

<5> Indicates how to provision and bind `PersistentVolumeClaims`. When not set, `VolumeBindingImmediate` is used. This field is only applied by servers that enable the `VolumeScheduling` feature.
<6> The {rh-virtualization} storage domain name to use.
<7> If `true`, the disk is thin provisioned. If `false`, the disk is preallocated. Thin provisioning is recommended.
<8> (Optional) File system type to be created. Possible values: `ext4` (default) or `xfs`.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/(Optional) File/Optional: File
This is how we generally introduce an optional step or value (with a colon).

Copy link
Contributor Author

@stoobie stoobie Aug 5, 2021

Choose a reason for hiding this comment

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

Fixed. The IBM SG actually says the same thing you did.
However, I found a lot of other instances of this in files that I don't own. I went ahead and made a few changes there too, even though those files are not connected to this PR. I can undo the changes if you like.

There are an additional 37 occurrences of (optional) in 19 files.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fantastic, thanks for taking care of all these.

@bobfuru bobfuru added the peer-review-done Signifies that the peer review team has reviewed this PR label Jul 26, 2021
@stoobie
Copy link
Contributor Author

stoobie commented Aug 5, 2021

@bobfuru I made the changes you suggested. Please approve and set this in the merge queue, or let me know what else should change.

Implemented SME comments.
Implemented QE commens.
Implemented Peer review comments.
@bobfuru
Copy link
Contributor

bobfuru commented Aug 5, 2021

LGTM - will merge/CP to 4.9

@bobfuru bobfuru merged commit 9852675 into openshift:main Aug 5, 2021
@bobfuru
Copy link
Contributor

bobfuru commented Aug 5, 2021

/cherrypick enterprise-4.9

@openshift-cherrypick-robot

@bobfuru: new pull request created: #35255

In response to this:

/cherrypick enterprise-4.9

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.

@stoobie stoobie deleted the 1980340 branch March 31, 2022 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.9 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.

None yet

6 participants