Skip to content

Conversation

wking
Copy link
Member

@wking wking commented Oct 8, 2020

xrefs aren't allowed in modules:

$ grep -A1 'You must not include xrefs' modules/mod-docs-ocp-conventions.adoc
You must not include xrefs in modules or create an xref to a module. You can
only use xrefs to link from one assembly to another.

And we can't move this module out into an assembly directory, because it is consumed from two places:

$ git --no-pager grep dynamic-provisioning-available-plugins
post_installation_configuration/storage-configuration.adoc:include::modules/dynamic-provisioning-available-plugins.adoc[leveloffset=+3]
storage/dynamic-provisioning.adoc:include::modules/dynamic-provisioning-available-plugins.adoc[leveloffset=+1]

We could:

a. Trim down post_installation_configuration to just xref the storage assembly, instead of duping so much of it's content via modules.
b. Drop the module and instead inline (some of) it with copy/paste in both assemblies.
c. Remove the xref.

With this commit, I'm going with (c), removing the xref which was added in 57961f1 (#22199). CC @bobfuru

xrefs aren't allowed in modules:

  $ grep -A1 'You must not include xrefs' modules/mod-docs-ocp-conventions.adoc
  You must not include xrefs in modules or create an xref to a module. You can
  only use xrefs to link from one assembly to another.

And we can't move this module out into an assembly directory, because
it is consumed from two places:

  $ git --no-pager grep dynamic-provisioning-available-plugins
  post_installation_configuration/storage-configuration.adoc:include::modules/dynamic-provisioning-available-plugins.adoc[leveloffset=+3]
  storage/dynamic-provisioning.adoc:include::modules/dynamic-provisioning-available-plugins.adoc[leveloffset=+1]

We could:

a. Trim down post_installation_configuration to just xref the storage
   assembly, instead of duping so much of it's content via modules.
b. Drop the module and instead inline (some of) it with copy/paste in
   both assemblies.
c. Remove the xref.

With this commit, I'm going with (c), removing the xref which was
added in 57961f1 ([STOR] Include section on the OpenStack Manila
CSI Driver, 2020-05-12, openshift#22199).
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 8, 2020
@wking
Copy link
Member Author

wking commented Oct 8, 2020

There's a similar entry in:

$ git --no-pager grep xref modules/dynamic-provisioning-manila-csi-definition.adoc
modules/dynamic-provisioning-manila-csi-definition.adoc:Once installed, the xref:../storage/container_storage_interface/persistent-storage-csi-manila.adoc#persistent-storage-csi-manila[OpenStack Manila CSI Driver Operator] and ManilaDriver automatically create the required storage classes for all available Manila share types needed for dynamic provisioning.

But it is not used by post_installation_configuration:

$ git --no-pager grep dynamic-provisioning-manila-csi-definition
storage/dynamic-provisioning.adoc:include::modules/dynamic-provisioning-manila-csi-definition.adoc[leveloffset=+2]

Is that oversight from post_installation_configuration? If not, we gain a new option:

d. Move the module under storage/ and keep the xref.

Thoughts?

@bobfuru
Copy link
Contributor

bobfuru commented Oct 8, 2020

There's a similar entry in:

$ git --no-pager grep xref modules/dynamic-provisioning-manila-csi-definition.adoc
modules/dynamic-provisioning-manila-csi-definition.adoc:Once installed, the xref:../storage/container_storage_interface/persistent-storage-csi-manila.adoc#persistent-storage-csi-manila[OpenStack Manila CSI Driver Operator] and ManilaDriver automatically create the required storage classes for all available Manila share types needed for dynamic provisioning.

But it is not used by post_installation_configuration:

$ git --no-pager grep dynamic-provisioning-manila-csi-definition
storage/dynamic-provisioning.adoc:include::modules/dynamic-provisioning-manila-csi-definition.adoc[leveloffset=+2]

Is that oversight from post_installation_configuration? If not, we gain a new option:

d. Move the module under storage/ and keep the xref.

Thoughts?

Thanks, @wking, for pointing this out. IIRC, the post_installation_configuration assembly was added before the xref'ed Manila doc was added, so it should also include the Manila entry. <-- @ahardin-rh Can you please confirm?

Good catch on the xref. I agree that it should be removed, so your proposed edit LGTM. Let me know if you'd like this PR merged, not sure if that option shows up for you in this repo? Thanks!

@wking
Copy link
Member Author

wking commented Oct 8, 2020

Let me know if you'd like this PR merged, not sure if that option shows up for you in this repo?

I can't merge in this repo, and even if I could, I'd rather not merge my own stuff ;). If this looks good to docs maintainers, they can merge when they're ready. I'll circle back to the dynamic-provisioning-manila-csi-definition.adoc xref if it doesn't get addressed by the change that updates post_installation_configuration.

@bobfuru
Copy link
Contributor

bobfuru commented Oct 8, 2020

If this looks good to docs maintainers, they can merge when they're ready. I'll circle back to the dynamic-provisioning-manila-csi-definition.adoc xref if it doesn't get addressed by the change that updates post_installation_configuration.

SGTM and LGTM, thanks again!

@bobfuru bobfuru merged commit 4167561 into openshift:master Oct 8, 2020
@bobfuru
Copy link
Contributor

bobfuru commented Oct 8, 2020

/cherrypick enterprise-4.5

@bobfuru
Copy link
Contributor

bobfuru commented Oct 8, 2020

/cherrypick enterprise-4.6

@openshift-cherrypick-robot

@bobfuru: new pull request created: #26273

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

@bobfuru: new pull request created: #26274

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 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants