Skip to content

Conversation

@bobfuru
Copy link
Contributor

@bobfuru bobfuru commented Oct 8, 2020

@bobfuru bobfuru added this to the Future Release milestone Oct 8, 2020
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 8, 2020
@openshift-docs-preview-bot

The preview will be available shortly at:

@bobfuru
Copy link
Contributor Author

bobfuru commented Oct 8, 2020

@marrusl PTAL

@bobfuru bobfuru force-pushed the update-var-section branch from 0f4c30e to 6d3a5f6 Compare October 8, 2020 23:17
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 8, 2020
@yuqi-zhang
Copy link

I think based on our conversations we need to modify some items here, including the whole machineconfig section. I can't really comment very easily on this PR since your changes only touches a few parts of the doc. So maybe a collaborative doc somewhere would be better, in any case:

  1. line 65: partitoin -> partition
  2. change all references from /var/log to /var. If we want to talk about /var/log and /var/lib/containers we should do that in separate sections
  3. line 145: /dev/nvme0n1 -> this is AWS only. This section is for baremetal, so we should at the very least leave this open as a variable with a footnote on it being the device name of the disk you want to partition
  4. line 148/149:
sizeMiB: 47000
startMiB: 47000

We should also make these variables, with a footnote that if you are to partition onto the same disk that sysroot in on, you must at least specify a startMiB (mebibytes) so that it doesn't stomp over your sysroot, as the default sysroot would normally grow to fit the reset of the disk

@bobfuru bobfuru force-pushed the update-var-section branch from 6d3a5f6 to 4279326 Compare October 9, 2020 20:34
@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 Oct 9, 2020
@bobfuru bobfuru force-pushed the update-var-section branch from 4279326 to b558e62 Compare October 9, 2020 20:37
@bobfuru
Copy link
Contributor Author

bobfuru commented Oct 9, 2020

I think based on our conversations we need to modify some items here, including the whole machineconfig section. I can't really comment very easily on this PR since your changes only touches a few parts of the doc. So maybe a collaborative doc somewhere would be better

Thanks for the comments, @yuqi-zhang - that's helpful. Yeah, I realized after creating the PR that commenting might be a challenge but kept it for now to be able to comment inline while seeing the diff.

I've applied your suggestions to the best of my understanding if you want to take a look. I also copied this section to a Google Doc in case it's easier to collaborate that way:
Creating a separate /var partition Both the Gdoc and the PR should have the same wording as of now.

If you need to edit more than what's in this procedure, feel free to add that in the Gdoc. Thanks!

Copy link
Contributor Author

@bobfuru bobfuru left a comment

Choose a reason for hiding this comment

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

@bgilbert Thanks for the comment. I am going to resolve the similar comment in https://github.com/openshift/openshift-docs/pull/25709/files#r498441768 and address the issue here.

@bobfuru bobfuru force-pushed the update-var-section branch from b558e62 to 94342b6 Compare October 14, 2020 01:20
@bobfuru bobfuru force-pushed the update-var-section branch from 94342b6 to bbdd3cb Compare October 14, 2020 22:44
@bobfuru
Copy link
Contributor Author

bobfuru commented Oct 14, 2020

Thanks for the review, @bgilbert. This will need to be merged tomorrow to make GA cutoff.

Could I please get a LGTM from your or @yuqi-zhang?

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

bobfuru commented Oct 14, 2020

@openshift/team-documentation PTAL

Copy link
Contributor

@sfortner-RH sfortner-RH left a comment

Choose a reason for hiding this comment

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

I had one small nit, but otherwise LGTM!

@bobfuru bobfuru force-pushed the update-var-section branch from bbdd3cb to 755794b Compare October 15, 2020 17:25
@bobfuru
Copy link
Contributor Author

bobfuru commented Oct 15, 2020

@bgilbert Made the last couple of changes. Can I please get your approval?

@bobfuru bobfuru 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 Oct 15, 2020
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.

LGTM!

@bobfuru bobfuru merged commit 8540319 into openshift:master Oct 15, 2020
@bobfuru
Copy link
Contributor Author

bobfuru commented Oct 15, 2020

/cherrypick enterprise-4.6

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Oct 15, 2020

@bobfuru: new pull request created: #26511

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.

@bobfuru bobfuru deleted the update-var-section branch October 15, 2020 18:16
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/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.

8 participants