BZ-1862278: Added changes as requested in the bug #31942
Conversation
openshift-ci-robot
added
the
size/M
|
Deploy preview for osdocs ready! Built with commit 8ff2668 |
|
Hi, #1 There probably should be a line break after As the rest is the output in separate lines like this: Please also remove the './" before the command to be consistent with rest of the oc commands on the page. #2 Currently, we have export COMPUTE_SUBNET= Both will work but not consistent. Please let me know what you think. |
| @@ -129,7 +129,7 @@ If you choose to create each rule based on events, you must create firewall rule | |||
| .Example output | |||
| [source,terminal] | |||
| ---- | |||
| Firewall change required by security admin: `gcloud compute firewall-rules create k8s-fw-a26e631036a3f46cba28f8df67266d55 --network example-network --description "{\"kubernetes.io/service-name\":\"openshift-ingress/router-default\", \"kubernetes.io/service-ip\":\"35.237.236.234\"}\" --allow tcp:443,tcp:80 --source-ranges 0.0.0.0/0 --target-tags exampl-fqzq7-master,exampl-fqzq7-worker --project example-project` | |||
| ./oc get events -n openshift-ingress --field-selector="reason=LoadBalancerManualChange" Firewall change required by security admin: `gcloud compute firewall-rules create k8s-fw-a26e631036a3f46cba28f8df67266d55 --network example-network --description "{\"kubernetes.io/service-name\":\"openshift-ingress/router-default\", \"kubernetes.io/service-ip\":\"35.237.236.234\"}\" --allow tcp:443,tcp:80 --source-ranges 0.0.0.0/0 --target-tags exampl-fqzq7-master,exampl-fqzq7-worker --project example-project` | |||
There was a problem hiding this comment.
+1 to @tsze-redhat's other related comment about a line break and dropping the ./. But further, if this addition is intending to illustrate what command you could run to view this example output, it seems like the command should also be in its own code block (with a $ prompt), prior to the example output block, to conform to our normal guidelines on showing commands with output. For example:
[source,terminal]
----
$ oc get events -n openshift-ingress --field-selector="reason=LoadBalancerManualChange"
---
.Example output
[source,terminal]
----
Firewall change required by security admin: `gcloud compute firewall-rules create k8s-fw-a26e631036a3f46cba28f8df67266d55 --network example-network --description "{\"kubernetes.io/service-name\":\"openshift-ingress/router-default\", \"kubernetes.io/service-ip\":\"35.237.236.234\"}\" --allow tcp:443,tcp:80 --source-ranges 0.0.0.0/0 --target-tags exampl-fqzq7-master,exampl-fqzq7-worker --project example-project`
----
Maybe also with a lead-in sentence before the oc get events command to provide some context for what's being shown. But if I'm misunderstanding and the literal example output includes the oc get events, then disregard.
| @@ -15,3 +15,5 @@ You can use the following Deployment Manager template to deploy the internal loa | |||
| include::https://raw.githubusercontent.com/openshift/installer/release-4.8/upi/gcp/02_lb_int.py[] | |||
| ---- | |||
| ==== | |||
|
|
|||
| You will need this in addition to the `02_lb_ext.py` when you create external cluster. | |||
There was a problem hiding this comment.
Possibly sprinkle "template" in a few places? Also seems to be missing the word "an" (or maybe "the") before "external cluster". E.g.:
You will need this template in addition to the `02_lb_ext.py` template when you create an external cluster.
| the ingress load balancer. You can create either a wildcard | ||
| `*.apps.{baseDomain}.` or specific records. You can use A, CNAME, and other | ||
| records per your requirements. | ||
| DNS Zone configuration is removed when Kubernetes manifests and generates Ignition configs. You must manually create DNS records that point at the ingress load balancer. You can create either a wildcard |
There was a problem hiding this comment.
It looks like in the rest of the assembly, it's mostly lowercase "zone" in "DNS zone". I do see the one uppercase instance in the Prerequisites list below here though, FWIW. Not sure which is accurate, but I would assume lowercase.
There was a problem hiding this comment.
I think the word "creating" might also be missing in the first sentence:
"DNS zone configuration is removed when creating Kubernetes manifests and generating Ignition configs."
| ifdef::shared-vpc[] | ||
| [NOTE] | ||
| ==== | ||
| This sections covers an optional step in other types of installation but it is mandatory for shared vpn install. Therefore, this change should NOT be propagated to other installation types. |
There was a problem hiding this comment.
Singular: s/sections/section/
s/installation but/installations, but/
There was a problem hiding this comment.
Is "vpn" in the BZ maybe a typo of "VPC"? So possibly:
s/vpn install/VPC installations/
There was a problem hiding this comment.
Re: "NOT", IBM SG states to avoid using uppercase for emphasis and allows italics. Our repo guidelines also allow italics, though suggests using sparingly.
There was a problem hiding this comment.
Re-reading the BZ now, actually, I'm wondering if this note was intended to be included verbatim in the doc, or was just describing the suggested paragraph change? @tsze-redhat Can you help confirm?
There was a problem hiding this comment.
Yes, 'vpn' should be 'vpc' - sorry. I edited the BZ.
And yes, that sentence beginning with 'Note:' is just a reminder that changes for the BZ are not supposed to go to other pages like Restricted Network or regular UPI install pages.
Hope this helps. Thanks.
There was a problem hiding this comment.
I removed this note from the documentation. I thought it was meant for the doc and not for us :-)
adellape
added
the
peer-review-done
|
LGTM. Thank you! |
|
/cherrypick enterprise-4.8 |
|
/cherrypick enterprise-4.7 |
|
/cherrypick enterprise-4.6 |
|
/cherrypick enterprise-4.5 |
|
@adellape: new pull request created: #32125 In response to this:
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. |
|
@adellape: new pull request created: #32126 In response to this:
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. |
|
@adellape: new pull request created: #32127 In response to this:
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. |
|
@adellape: new pull request created: #32128 In response to this:
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. |
Applies to 4.5+
BZ Link: https://bugzilla.redhat.com/show_bug.cgi?id=1862278 [Please see the BZ Link as the requested changes are in Comment 0]
Preview Links:
https://deploy-preview-31942--osdocs.netlify.app/openshift-enterprise/latest/installing/installing_gcp/installing-gcp-user-infra-vpc.html#installation-gcp-user-infra-shared-vpc-config-yaml_installing-gcp-user-infra-vpc [Revised build system to make Guard useful. #5 on the install-config.yaml file]
https://deploy-preview-31942--osdocs.netlify.app/openshift-enterprise/latest/installing/installing_gcp/installing-gcp-user-infra-vpc.html#installation-deployment-manager-int-lb_installing-gcp-user-infra-vpc [After the 02_lb_int.py template file. Added a paragraph]
https://deploy-preview-31942--osdocs.netlify.app/openshift-enterprise/latest/installing/installing_gcp/installing-gcp-user-infra-vpc.html#installation-creating-gcp-worker_installing-gcp-user-infra-vpc [Step 2a]
https://deploy-preview-31942--osdocs.netlify.app/openshift-enterprise/latest/installing/installing_gcp/installing-gcp-user-infra-vpc.html#installation-gcp-user-infra-adding-ingress_installing-gcp-user-infra-vpc [First paragraph + new note]
https://deploy-preview-31942--osdocs.netlify.app/openshift-enterprise/latest/installing/installing_gcp/installing-gcp-user-infra-vpc.html#installation-gcp-user-infra-vpc-adding-firewall-rules [Example Output]
Pending QE Approval: @tsze-redhat