Skip to content

Comments

Bug 1306520 Document persistent volume storage labeling#1744

Merged
tnguyen-rh merged 1 commit intoopenshift:masterfrom
tnguyen-rh:bz1306520
Mar 18, 2016
Merged

Bug 1306520 Document persistent volume storage labeling#1744
tnguyen-rh merged 1 commit intoopenshift:masterfrom
tnguyen-rh:bz1306520

Conversation

@tnguyen-rh
Copy link

@tnguyen-rh tnguyen-rh added this to the Future Release milestone Mar 15, 2016
@tnguyen-rh tnguyen-rh added tech_review peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Mar 15, 2016
Copy link
Contributor

Choose a reason for hiding this comment

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

"Term for Key/Value Pairs" and "Storage Type" are not showing up as bold headers here, though the "Project Name/PVC Name" headers in the table below are behaving properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tnguyen-rh , just one comment. Other than that, LGTM! 🎆

@tnguyen-rh
Copy link
Author

@ahardin-rh Thanks. I've removed the (dubious) options="headers" part and moved things around. The table header line is now bold, as expected.

Choose a reason for hiding this comment

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

I'm not sure that these commas are necessary.

Copy link
Author

Choose a reason for hiding this comment

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

@tpoitras
Yeah, i think the sentence works w/o those commas, as well.

@tpoitras
Copy link

@tnguyen-rh made some suggestions 👍

Choose a reason for hiding this comment

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

"The term storage-provisioner label is what this documentation uses." Probably don't need to mention this here and above. Maybe get rid of it above, and keep it here, as we're about to talk about the table?

"This document uses the term storage-provisioner label in place of the three key/value pairs."

Also, is "understand" the right word? Maybe something like "use"?

Copy link
Author

Choose a reason for hiding this comment

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

@bfallonf
Right, "understand" is not the best choice.

Hmm, getting rid of storage-provisioner label entirely is a good idea.

@bfallonf
Copy link

A few nits as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

"associated with OpenShift" - I would use "dynamically created by OpenShift" to emphasize that we touch only volumes created by dynamic provisioning.

@jsafrane
Copy link
Contributor

One minor nit, otherwise the documentation looks fine from technical point of view. I leave the style and exact wording for others to review.

@tnguyen-rh
Copy link
Author

@jsafrane
Thanks for the review.

One last technical wrinkle: I invented the generic term storage-provisioner labels but am not completely happy w/ it, and would really prefer to use a term that OpenShift developers use or will use. I remember from our meeting that there is no such term at present. So the following questions are mostly a thought-experiment:

  • Do you think storage-provisioner labels has a chance of being accepted (and used) by the OS devs?
  • If yes, could you please start to use it in the code (e.g., in comments)?
  • If maybe, what would be the objections?
  • If no, what do you think will be the "winning" term, eventually?

@jsafrane
Copy link
Contributor

In the code, we use term 'cloud tags' and 'Tags to attach to the real volume in the cloud provider - e.g. AWS EBS'. I don't think this should be the same in the documentation.

@tnguyen-rh
Copy link
Author

@bfallonf @tpoitras
I've chosen another term, Volume Owner Information, and have reorganized a bit. Better?

Copy link
Contributor

Choose a reason for hiding this comment

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

s/visibile/visible/

Copy link
Author

Choose a reason for hiding this comment

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

Che imabarazzo...

@adellape Thanks.

@bfallonf
Copy link

@tnguyen-rh LGTM 👍

@tnguyen-rh tnguyen-rh removed peer-review-in-progress Signifies that the peer review team is reviewing this PR tech_review labels Mar 18, 2016
@tnguyen-rh
Copy link
Author

[rev_history]
|link:../install_config/persistent_storage/dynamically_provisioning_pvs.html[Configuring Persistent Storage -> Dynamic Provisioning]
|Documented link:../install_config/persistent_storage/dynamically_provisioning_pvs.html#volume-owner-info[Volume Owner Information].
%
|link:../install_config/persistent_storage/dynamically_provisioning_aws.html[Configuring Persistent Storage -> Persistent Storage Using AWS Elastic Block Store]
|Linked to link:../install_config/persistent_storage/dynamically_provisioning_pvs.html#volume-owner-info[Volume Owner Information].
%
|link:../install_config/persistent_storage/dynamically_provisioning_cinder.html[Configuring Persistent Storage -> Persistent Storage Using OpenStack Cinder]
|Linked to link:../install_config/persistent_storage/dynamically_provisioning_pvs.html#volume-owner-info[Volume Owner Information].
%
|link:../install_config/persistent_storage/dynamically_provisioning_gce.html[Configuring Persistent Storage -> Persistent Storage Using GCE Persistent Disk]
|Linked to link:../install_config/persistent_storage/dynamically_provisioning_pvs.html#volume-owner-info[Volume Owner Information].
%

tnguyen-rh added a commit that referenced this pull request Mar 18, 2016
Bug 1306520 Document persistent volume storage labeling
@tnguyen-rh tnguyen-rh merged commit e5e8654 into openshift:master Mar 18, 2016
@tnguyen-rh tnguyen-rh deleted the bz1306520 branch March 18, 2016 12:34
@adellape adellape modified the milestones: OSE 3.2, Future Release May 3, 2016
@adellape adellape modified the milestones: OSE 3.2, Staging, OSE 3.2 (Picked) May 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants