Skip to content

Conversation

@bobfuru
Copy link
Contributor

@bobfuru bobfuru commented Sep 22, 2020

@bobfuru bobfuru added this to the Future Release milestone Sep 22, 2020
@bobfuru bobfuru requested a review from chrisnegus September 22, 2020 00:22
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 22, 2020
@openshift-docs-preview-bot

The preview will be available shortly at:

@bobfuru bobfuru force-pushed the OSDOCS-1496 branch 11 times, most recently from e043d05 to 93e9005 Compare September 25, 2020 14:45
Copy link
Contributor

@chrisnegus chrisnegus left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Bob!

@bobfuru bobfuru added the peer-review-needed Signifies that the peer review team needs to review this PR label Sep 25, 2020
Copy link
Contributor

@sheriff-rh sheriff-rh left a comment

Choose a reason for hiding this comment

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

Nice tables! LGTM! 🎉 🌮 🥇

@sheriff-rh sheriff-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 Sep 25, 2020
@bobfuru
Copy link
Contributor Author

bobfuru commented Sep 25, 2020

This will be merged to master and CP'ed to 4.6 so that it can be viewed and reviewed in the context of it's parent assembly, #25709.

Note that this PR 25656 has been reviewed by @chrisnegus but not reviewed by any other RHCOS SMEs.

@bobfuru bobfuru merged commit c661a85 into openshift:master Sep 25, 2020
@bobfuru
Copy link
Contributor Author

bobfuru commented Sep 25, 2020

/cherrypick enterprise-4.6

@openshift-cherrypick-robot

@bobfuru: new pull request created: #25813

Details

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.


a|`coreos.inst.ignition_url`

a|Optional: The URL of the Ignition config. If no URL is specified, no Ignition config will be embedded.
Copy link
Member

Choose a reason for hiding this comment

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

How about

a|Optional: The URL of the Ignition config to embed into the installed system. If no URL is specified, no Ignition config will be embedded.

? To make it clear it's not the Ignition config of the live boot itself.


a|`coreos.inst.image_url`

a|Optional: Download and install the specified {op-system} image, overriding `coreos.inst.stream`.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not sure if it's worth mentioning coreos.inst.stream here since it's not in the table itself, and it's not relevant to RHCOS/OCP. (Possibly OKD... though not sure there either.)

If we want to keep it, maybe let's make it clear that it overrides the default "built-in" image. E.g.

a|Optional: Download and install the specified {op-system} image instead of the one embedded in the live media.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I missed this mention of coreos.inst.stream and agree that it makes sense to remove it here.


* This argument should not be used in production environments and is intended for debugging purposes only.

* You must use this option if you were already using the previous `coreos-installer`.
Copy link
Member

Choose a reason for hiding this comment

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

  • You must use this option if you were already using the previous coreos-installer.

I'm wondering if we even need to mention this. Aren't these docs versioned to the OCP version already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as there's nothing that an existing user must actually do here when they upgrade from a previous version of OCP, then I agree it should be left out. Removed.


a|`coreos.inst.platform_id`

a| Optional: The Ignition platform ID of the platform the {op-system} image is being installed on. Default is bare metal. This option determines whether or not to request an Ignition config from the cloud provider, such as AWS or WMware. For example: `coreos.inst.platform_id=vmware`.
Copy link
Member

Choose a reason for hiding this comment

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

I'm debating whether we should include this at all. It's really a corner-case users shouldn't normally hit. Maybe let's put it near the bottom instead? /cc @bgilbert WDYT?

Also "Default is bare metal" -> "Default is metal" . And WMware -> VMware. I'd also drop the AWS example since it's not relevant for 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.

Ack. Fixed the spelling errors and moved to the bottom of the table.

Copy link
Contributor

Choose a reason for hiding this comment

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

VMware UPI should set this, so I'm +1 to leaving it in the doc.

@bobfuru
Copy link
Contributor Author

bobfuru commented Sep 30, 2020

@jlebon Thanks for the review and feedback! This PR had already been merged by the time you submitted your review, so I've addressed your comments in a new PR: #26004. I'm going to get that new PR merged so that the changes are picked up by the overarching coreos-installer assembly in #25709. 🎉

bobfuru pushed a commit to bobfuru/openshift-docs that referenced this pull request Sep 30, 2020
bobfuru pushed a commit that referenced this pull request Sep 30, 2020
Fixes to PR #25656 - applies SME feedback
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/openshift-docs that referenced this pull request Sep 30, 2020
bobfuru pushed a commit that referenced this pull request Sep 30, 2020
…-26004-to-enterprise-4.6

[enterprise-4.6] Fixes to PR #25656 - applies SME feedback
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

Apologies for the late review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.6 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

Development

Successfully merging this pull request may close these issues.

9 participants