Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OSDOCS-3939: [GCP] Enable user specified networking tags #50178

Merged
merged 1 commit into from Oct 7, 2022
Merged

OSDOCS-3939: [GCP] Enable user specified networking tags #50178

merged 1 commit into from Oct 7, 2022

Conversation

bscott-rh
Copy link
Contributor

@bscott-rh bscott-rh commented Sep 8, 2022 •
edited

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 8, 2022
@bscott-rh bscott-rh force-pushed the OSDOCS-3939 branch 2 times, most recently from fa38803 to 4b54011 Compare September 12, 2022 19:54
@ocpdocs-previewbot
Copy link

🤖 Bots are busy building the preview. It will be available soon at:
https://50178--docspreview.netlify.app

@bscott-rh bscott-rh mentioned this pull request Sep 12, 2022
Closed
@bscott-rh bscott-rh force-pushed the OSDOCS-3939 branch 2 times, most recently from 84515eb to 275db68 Compare September 12, 2022 20:52
@bscott-rh
Copy link
Contributor Author

@jianli-wei Please review these changes for CORS-2209. There are 3 new parameters that were added to the install-config table as well as the sample yaml file. Thank you

@jianli-wei
Copy link

@bscott-rh The changes look good, but some suggestions on the Table 4, see the sample sheet.

@bscott-rh
Copy link
Contributor Author

@bscott-rh The changes look good, but some suggestions on the Table 4, see the sample sheet.

@jianli-wei I have updated Table 4 to incorporate the values that were missing from the sample sheet. Please take a look and let me know if I've caught everything. Thank you

jianli-wei
jianli-wei suggested changes Sep 20, 2022
View reviewed changes
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
@bscott-rh
Copy link
Contributor Author

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Sep 22, 2022
@maxwelldb maxwelldb added peer-review-in-progress Signifies that the peer review team is reviewing this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 22, 2022
@maxwelldb maxwelldb self-requested a review September 22, 2022 19:51
@maxwelldb maxwelldb added this to the Planned for 4.12 GA milestone Sep 22, 2022
maxwelldb
maxwelldb reviewed Sep 22, 2022
View reviewed changes
Copy link
Contributor

@maxwelldb maxwelldb left a comment

Choose a reason for hiding this comment

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

Lookin' pretty good. Made some comments, most suggestions or advisories.

If you have time, consider clawing as many future tense statements back to present whenever you can.

modules/installation-configuration-parameters.adoc
@@ -232,7 +232,7 @@ Required installation configuration parameters are described in the following ta
|Parameter|Description|Values
Copy link
Contributor

Choose a reason for hiding this comment

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

This table seems suuuuuuuuper wide. This is not the case on prod, so might want to track down what's causing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is due to the addition of some super long parameters like platform.gcp.defaultMachinePlatform.osDisk.encryptionKey.kmsKeyServiceAccount. Is there a way to change the table's wrapping behavior? Otherwise we will need to rethink how we list these deeply nested parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unaware of a simple way to do this with adoc. You could play with the various table and cell width options, though you'd need to verify that the table looks good on both the portal and d.o.c. 😨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving this as it is until we are able to refactor/revise the installation configuration parameters as a whole. I think this will be a good continuous improvement initiative.

modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-configuration-parameters.adoc Outdated Show resolved Hide resolved
modules/installation-gcp-config-yaml.adoc Show resolved Hide resolved
modules/installation-gcp-user-infra-shared-vpc-config-yaml.adoc Outdated Show resolved Hide resolved
@maxwelldb maxwelldb added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Sep 22, 2022
@bscott-rh
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Oct 6, 2022
@mburke5678 mburke5678 merged commit ca8f290 into openshift:main Oct 7, 2022
@mburke5678
Copy link
Contributor

/cherrypick enterprise-4.12

@mburke5678 mburke5678 removed the merge-review-needed Signifies that the merge review team needs to review this PR label Oct 7, 2022
@openshift-cherrypick-robot
Copy link

@mburke5678: new pull request created: #51401

In response to this:

/cherrypick enterprise-4.12

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 openshift-cherrypick-robot mentioned this pull request Oct 7, 2022
Merged
@bscott-rh bscott-rh mentioned this pull request Oct 12, 2022
Merged
1 task
@bscott-rh bscott-rh mentioned this pull request Feb 10, 2023
Merged
@bscott-rh bscott-rh deleted the OSDOCS-3939 branch October 30, 2023 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxwelldb maxwelldb maxwelldb left review comments

@jianli-wei jianli-wei jianli-wei requested changes

Assignees

@bscott-rh bscott-rh

Labels
branch/enterprise-4.12 peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
OCP 4.12 GA
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bscott-rh @ocpdocs-previewbot @jianli-wei @mburke5678 @openshift-cherrypick-robot @maxwelldb