Skip to content

Bug 1591941 Added footnotes about cidr range for egress_source value#10591

Merged
bfallonf merged 1 commit intoopenshift:masterfrom
bfallonf:cidr_1527602
Jul 12, 2018
Merged

Bug 1591941 Added footnotes about cidr range for egress_source value#10591
bfallonf merged 1 commit intoopenshift:masterfrom
bfallonf:cidr_1527602

Conversation

@bfallonf
Copy link

@bfallonf bfallonf commented Jul 3, 2018

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 3, 2018
@bfallonf bfallonf requested a review from danwinship July 3, 2018 03:56
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would probably be better to change the examples to use the CIDR form, because it's not really that hard for the user to do, and it results in fewer surprises.

So change the EGRESS_SOURCE value above in the yaml to 192.168.12.99/24, and then change the footnote here to something like:

IP address from the physical network that the node is on and is reserved by the cluster administrator for use by this pod. For best results, the IP address should include the subnet length (eg, the /24 here) so that a proper route to the local subnet can be set up. (If you do not include a subnet length then the egress router will not be able to access other hosts on its subnet, besides the EGRESS_GATEWAY host.)

Then in the following examples that don't currently have EGRESS_SOURCE footnotes, just add the /24, and in the ones that do have EGRESS_SOURCE footnotes, copy the same text as you put here

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 10, 2018
@bfallonf bfallonf added the peer-review-needed Signifies that the peer review team needs to review this PR label Jul 10, 2018
@bfallonf
Copy link
Author

Thanks, @danwinship . I've updated to your suggestion, with a bit of rewording (mainly to get away from the brackets and to highlight the optional-but-recommended-ness of it). If you have more thoughts, please let me know.

@openshift/team-documentation PTAL

Copy link
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

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

A couple of style picks and suggestions, including one for a line you didn't actually change (sorry!). This sounds good. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd s/and is reserved/and that is reserved

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid parentheses, s/length (the /24)/length, the /24 suffix,
s/set up/set

Copy link
Contributor

Choose a reason for hiding this comment

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

s/host/host that is
no comma

Copy link
Contributor

Choose a reason for hiding this comment

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

same here :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I might move the annotation up to the same line as EGRESS_DESTINATION and say something like "<2> Provide EGRESS_DESTINATION values as multi-line YAML string. Use the following formats:
ul goes here"

@kalexand-rh kalexand-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Jul 11, 2018
@bfallonf bfallonf merged commit 56751af into openshift:master Jul 12, 2018
@bfallonf
Copy link
Author

/cherrypick enterprise-3.10

@openshift-cherrypick-robot

@bfallonf: new pull request created: #10764

Details

In response to this:

/cherrypick enterprise-3.10

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.

@bfallonf
Copy link
Author

/cherrypick enterprise-3.9

@openshift-cherrypick-robot

@bfallonf: #10591 failed to apply on top of branch "enterprise-3.9":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	admin_guide/managing_networking.adoc
Falling back to patching base and 3-way merge...
Auto-merging admin_guide/managing_networking.adoc
CONFLICT (content): Merge conflict in admin_guide/managing_networking.adoc
Patch failed at 0001 added footnotes about cidr range for egress_source value

Details

In response to this:

/cherrypick enterprise-3.9

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-3.9 branch/enterprise-3.10 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.

5 participants