Skip to content

Conversation

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 16, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Jan 16, 2023

🤖 Updated build preview is available at:
https://54711--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/15969

@alishaIBM
Copy link
Contributor Author

/assign @Madhan-SWE

@alishaIBM alishaIBM changed the title [WIP] IBM Power VS Block CSI Driver Operator [WIP] IBM Power Virtual Server Block CSI Driver Operator Mar 1, 2023
@alishaIBM alishaIBM force-pushed the csi branch 2 times, most recently from 4e8ecdb to 25f994e Compare March 1, 2023 08:55
@alishaIBM alishaIBM force-pushed the csi branch 2 times, most recently from cce02a6 to ebbcf34 Compare March 20, 2023 06:06
@alishaIBM
Copy link
Contributor Author

@Madhan-SWE ,
I have incorporated the changes. Request you to review and provide QE approval. Thanks.

@alishaIBM alishaIBM changed the title [WIP] IBM Power Virtual Server Block CSI Driver Operator IBM Power Virtual Server Block CSI Driver Operator Mar 20, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2023
@Madhan-SWE
Copy link

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2023
@alishaIBM
Copy link
Contributor Author

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Mar 21, 2023
@kelbrown20 kelbrown20 added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Mar 21, 2023
Copy link
Contributor

@kelbrown20 kelbrown20 left a comment

Choose a reason for hiding this comment

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

Hello! Great work! Just some small questions and considerations with formating. Thank you!

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 31, 2023
{product-title} is capable of provisioning persistent volumes (PVs) using the Container Storage Interface (CSI) driver for {ibmpowerProductName} Virtual Server Block Storage.

:FeatureName: {ibmpowerProductName} Virtual Server Block CSI Driver Operator
include::snippets/technology-preview.adoc[leveloffset=+1]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added Technology Preview Note. FYI : @manojnkumar @Madhan-SWE @kelbrown20

@alishaIBM
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Apr 4, 2023
@maxwelldb maxwelldb added the merge-review-in-progress Signifies that the merge review team is reviewing this PR label Apr 4, 2023
@maxwelldb maxwelldb self-requested a review April 4, 2023 18:03
Copy link
Contributor

@maxwelldb maxwelldb left a comment

Choose a reason for hiding this comment

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

Made some suggestions as part of merge review. Feel free to re-add the label when you're ready.

Copy link
Contributor

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

This documentation follows the same format as every other csi driver operator in the OpenShift docs.
Ref: https://docs.openshift.com/container-platform/4.8/storage/container_storage_interface/persistent-storage-csi-gcp-pd.html

Do we need to put additional introduction info?

Copy link

@Madhan-SWE Madhan-SWE Apr 20, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor

@maxwelldb maxwelldb Apr 20, 2023

Choose a reason for hiding this comment

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

@Madhan-SWE I'd say that those docs should also follow the format in our guidelines. Can't explain why they don't. There could be an exception for these assemblies, but I don't have a great way to find that out.

e: I'm not on a review squad this week, so I'm sure someone will pick this up shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxwelldb thanks for the update.

@kelbrown20 @maxwelldb could you please point us to the right person who can take a look at above. Thanks.
cc : @jaypoulz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@sferich888, since these are all within the openshift org, are you ok with them? Or are there specific requirements you'd like to be confirmed first?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not link to GitHub no matter where the content is.

If we are providing a link for a purpose then the content of that link should be documented in our docs.

Most of these looks like references (to whit I have to ask the value of the reference), shot of providing a breadcrumb trail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new content (without GitHub links).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kelbrown20 @kalexand-rh could you please review the PR content ? Thanks.

:FeatureName: {ibmpowerProductName} Virtual Server Block CSI Driver Operator
include::snippets/technology-preview.adoc[leveloffset=+1]

Familiarity with xref:../../storage/understanding-persistent-storage.adoc#understanding-persistent-storage[persistent storage] and xref:../../storage/container_storage_interface/persistent-storage-csi.adoc#persistent-storage-csi[configuring CSI volumes] is recommended when working with a CSI Operator and driver.
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ ISG: Could revise to eliminate passive voice and use of "recommended."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced "recommended" with "helpful"


* The _{ibmpowerProductName} Virtual Server Block CSI Driver Operator_ provides two storage classes named `ibm-powervs-tier1` (default), and `ibm-powervs-tier3` for different tiers that you can use to create persistent volume claims (PVCs). The {ibmpowerProductName} Virtual Server Block CSI Driver Operator supports dynamic volume provisioning by allowing storage volumes to be created on demand, eliminating the need for cluster administrators to pre-provision storage.

* The _{ibmpowerProductName} Virtual Server Block CSI driver_ enables you to create and mount {ibmpowerProductName} Virtual Server Block PVs.
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ ISG: Anthropomorphization in "enables".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced "enables" with "allows"

@maxwelldb maxwelldb removed merge-review-in-progress Signifies that the merge review team is reviewing this PR merge-review-needed Signifies that the merge review team needs to review this PR labels Apr 4, 2023
@alishaIBM alishaIBM force-pushed the csi branch 2 times, most recently from 1e47e1b to 3ea9be6 Compare April 7, 2023 15:21
@alishaIBM alishaIBM force-pushed the csi branch 2 times, most recently from 263e5f6 to 4407778 Compare May 8, 2023 15:07
@alishaIBM
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label May 8, 2023
@sheriff-rh sheriff-rh added merge-review-in-progress Signifies that the merge review team is reviewing this PR and removed merge-review-needed Signifies that the merge review team needs to review this PR labels May 9, 2023
Copy link
Contributor

@sheriff-rh sheriff-rh left a comment

Choose a reason for hiding this comment

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

Looks good Alisha! I have a couple of observations before we can merge.


toc::[]

:_content-type: CONCEPT
Copy link
Contributor

Choose a reason for hiding this comment

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

According to our guidelines, the content-type needs to be on line 1. Also, this needs to be ASSEMBLY not CONCEPT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed :
:_content-type: CONCEPT

@sheriff-rh please check if correct. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alishaIBM content-type needs to be on line 1 and it needs to be ASSEMBLY type:

:_content-type: ASSEMBLY
[id="persistent-storage-csi-ibm-powervs-block"]
= {ibmpowerProductName} Virtual Server Block CSI Driver Operator
include::_attributes/common-attributes.adoc[]
:context: persistent-storage-csi-ibm-powervs-block

toc::[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sheriff-rh
Incorporated above suggestion. Thanks.


The IBM Power Virtual Server Block CSI Driver will be installed through IBM Power Virtual Server Block CSI Driver Operator and the operator is based on libarary-go. The OpenShift library-go is a collection of functions that allow us to build OpenShift operators easily. Most of the functionality of a CSI driver operator is already available there. The IBM Power Virtual Server Block CSI Driver Operator is installed by the cluster-storage-operator. The Cluster-storage-operator installs the IBM Power Virtual Server Block CSI Driver Operator if the Platform type is Power Virtual Servers.

== Overview
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs an [id=] block before Overview.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an [id=] block before Overview.

@sheriff-rh sheriff-rh removed the merge-review-in-progress Signifies that the merge review team is reviewing this PR label May 9, 2023
@alishaIBM alishaIBM force-pushed the csi branch 2 times, most recently from 3e2452f to 2a99d89 Compare May 10, 2023 07:12
@alishaIBM
Copy link
Contributor Author

@sheriff-rh I have addressed the comments. Request you to please review the same. Thanks.

@openshift-ci
Copy link

openshift-ci bot commented May 10, 2023

@ocpdocs-previewbot: user ocpdocs-previewbot is not trusted for pull request #54711

Copy link
Contributor

@sheriff-rh sheriff-rh left a comment

Choose a reason for hiding this comment

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

Feedback incorporated, LGTM! Merging.

@sheriff-rh sheriff-rh merged commit 3c0c573 into openshift:main May 10, 2023
@sheriff-rh
Copy link
Contributor

/cherrypick enterprise-4.13

@openshift-cherrypick-robot

@sheriff-rh: new pull request created: #59786

Details

In response to this:

/cherrypick enterprise-4.13

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.13 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.

10 participants