Skip to content

Conversation

@JStickler
Copy link
Contributor

@JStickler JStickler commented May 19, 2022

Version(s): 4.6 - 4.11

Issue: OSSMDOC-525

Link to docs preview: No visible changes in output?

This PR is not intended to address all required Jupiter changes, it only includes some preliminary cleanup done while scoping Jupiter work for the Epic OSSMDOC-523. This PR may include:

  • updating IDs to match headings
  • Deleting comments or notes
  • replacing text with attributes
  • Adding empty lines required by formatting
  • Deleting double spaces after a period

Copy link
Contributor

@pneedle-rh pneedle-rh left a comment

Choose a reason for hiding this comment

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

@JStickler hi! I have completed my review of this PR. Please let me know if you have any questions.

Please provide a link to a locally-generated preview after you have addressed my suggestions. The changes in this PR do make a difference to the preview build and as such I cloned your fork locally and generated a preview in order to complete my review.

Please also squash the commits into one.

Thanks!

:_content-type: PROCEDURE
[id="ossm-security-mtls-sidecars-outgoing_{context}"]
== Configuring sidecars for outgoing connections
= Configuring sidecars for outgoing connections
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to highlight that with this line change, the section heading is rendered visible in the TOC in the ../service_mesh/v2x/ossm-security.html assembly. In the live version is an H3 heading embedded within the H2 "Enabling strict mTLS across the service mesh" heading. But, with this change, in the preview it becomes an H2 heading that is parallel with "Enabling strict mTLS across the service mesh", not embedded within it.

Please create a local preview and review all of the heading changes that you have made in this PR to see if the indentations are changed. If they are, you might need to increment the level offset in the corresponding include statements in the assemblies that include the modules so that the headings remain at the same indentation that they are in the live documentation.

Essentially, as it stands this change moves the heading up one indentation because the corresponding include has not been incremented to allow for the adjustment from == to =. This is the case for several of these in this PR, but I will not mention them all here.

Please also check your other Jupiter update PRs for similar occurrences.

@@ -0,0 +1,11 @@
// Module included in the following assemblies:
//
// * service_mesh/v2x/customizing-installation-ossm.adoc
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see the assembly mentioned in this line in the file structure in this PR. This module also does not appear to be added anywhere else either. Are you intending to include it in this PR or in a later one?



[id="ossm-specifying-jaeger-configuration_{context}"]
= Specifying Jaeger configuration in the SMCP
Copy link
Contributor

Choose a reason for hiding this comment

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

You could update "Jaeger configuration" to "a Jaeger configuration" or something similar.

//
// * service_mesh/v2x/customizing-installation-ossm.adoc


Copy link
Contributor

Choose a reason for hiding this comment

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

Each .adoc file must contain a :_content-type: attribute in its metadata that indicates its file type. Please add one in this file. Please see https://github.com/openshift/openshift-docs/blob/main/contributing_to_docs/doc_guidelines.adoc#content-type-attributes for more details.

[id="ossm-specifying-jaeger-configuration_{context}"]
= Specifying Jaeger configuration in the SMCP

You configure Jaeger under the `addons` section of the `ServiceMeshControlPlane` resource. However, there are limitations to what you can configure in the SMCP.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could define the full description of "SMCP" directly before the first use of this acronym in this file.

== Additional resources

* For more information about tuning {SMProductShortName} for performance, see xref:../../service_mesh/v2x/ossm-performance-scalability.adoc#ossm-performance-scalability[Performance and scalability].
* xref:../../service_mesh/v2x/ossm-performance-scalability.adoc#ossm-performance-scalability[Performance and scalability].
Copy link
Contributor

Choose a reason for hiding this comment

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

You could remove the full stop from this bullet point, since it is not a full sentence.


* Install and configure {SMProductName}.
* Test your configuration in a staging environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

The TOC is not rendered for this page in the preview. Please add a blank link prior to the toc::[] line at line 6 and check if that then resolves the issue. I am unable to comment directly in that line in this PR.

include::modules/ossm-config-sec-mtls-mesh.adoc[leveloffset=+2]

include::modules/ossm-config-sidecar-mtls.adoc[leveloffset=+3]
include::modules/ossm-config-sidecar-mtls.adoc[leveloffset=+2]
Copy link
Contributor

Choose a reason for hiding this comment

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

In the live build, the first heading in the target module is H3 in the assembly and it is subsequently not rendered in the TOC. Are you intending to increase it by one level? In the preview, the "Configuring sidecars for incoming connections for specific services" heading is parallel to the "Enabling strict mTLS across the service mesh" heading, whereas in the live build it is nested within it.

==== Additional resources

[role="_additional-resources"]
== Additional resources
Copy link
Contributor

Choose a reason for hiding this comment

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

You could update this to .Additional resources, since there are subsequent module includes and this seems specific to the preceding module.


[role="_additional-resources"]
.Additional resources
== Additional resources
Copy link
Contributor

Choose a reason for hiding this comment

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

You could keep this as .Additional resources, since there are subsequent module includes and this seems specific to the preceding module.

@pneedle-rh pneedle-rh added the peer-review-done Signifies that the peer review team has reviewed this PR label May 25, 2022
@pneedle-rh pneedle-rh added this to the Next Release milestone May 25, 2022
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2022
@openshift-ci
Copy link

openshift-ci bot commented Jun 25, 2022

@JStickler: PR needs rebase.

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.

@bergerhoffer
Copy link
Contributor

The enterprise-4.12 label has been added to this PR.

This is because your PR targets the main branch and is labeled for enterprise-4.11. And any PR going into main must also target the latest version branch (enterprise-4.12).

If the update in your PR does NOT apply to version 4.12 onward, please re-target this PR to go directly into the appropriate version branch or branches (enterprise-4.x) instead of main.

@JStickler
Copy link
Contributor Author

Closing - outdated.

@JStickler JStickler closed this Oct 12, 2022
@JStickler JStickler deleted the OSSMDOC-525A branch May 22, 2023 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.6 branch/enterprise-4.7 branch/enterprise-4.8 branch/enterprise-4.9 branch/enterprise-4.10 branch/enterprise-4.11 branch/enterprise-4.12 needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. peer-review-done Signifies that the peer review team has reviewed this PR service-mesh Label for all Service Mesh PRs size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants