Skip to content

Conversation

@vikram-redhat
Copy link
Contributor

@vikram-redhat vikram-redhat commented Oct 15, 2020

@codyhoag now that https://github.com/openshift/openshift-docs/pull/26184/files has merged into the enterprise-4.6 branch, I am adding that into master. There were some conflicts, which I think I have resolved correctly, but please double check and merge if ok. You should be able to push this branch if there are any issues. Sorry I know it's extra work for you, but this one was very tricky, and I want to make sure I got this right so future updates can be done for Power and Z docs directly in master.

@bobfuru I was unsure of the fixes I had to do due to this line that you recently updated: vikram-redhat@195ef0d#diff-812300ac04e88a61895332850e1e5f42004729ad650880e5cdf0b4b57f0490f1R56. These are here in this PR: https://github.com/openshift/openshift-docs/pull/26490/files#diff-812300ac04e88a61895332850e1e5f42004729ad650880e5cdf0b4b57f0490f1R56. Could you please double check it looks ok?

Thanks!

@SNiemann15 and @ktania46 - FYI. Future updates should be done in master.

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 15, 2020
Copy link
Contributor

@codyhoag codyhoag left a comment

Choose a reason for hiding this comment

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

I found only one issue. I will make the change and force push.


a|Specify multiple network interfaces by specifying multiple `ip=` entries.

|Specify multiple network interfaces by specifying multiple `ip=` entries.
Copy link
Contributor

Choose a reason for hiding this comment

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

This file contained the only differences I found between the enterprise-4.6 PR and this one. Specifically, this block of changes was missing. These changes should be on lines 10-33 of this file (above what's highlighted here).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @codyhoag. This change confuses me because it rewrites what was there before and I don't know what ifdefs to apply when I add the missing content back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Upon further review, I don't actually think ifdefs are necessary. Just a restore to the original content in this file.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 15, 2020
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 15, 2020
@codyhoag
Copy link
Contributor

@ktania46 can you review aae3b65a146016e3e4806978eec68c645db2237a (the second commit I made to make this master PR identical with the 4.6 PR) and make sure that's correct? I believe that content was being used by the bare metal installer docs, but wanted to check the rationale behind the edits.

If those edits are not needed, I can revert that commit. That change will also need to be made for 4.6 too, if that's the case, so the content is identical. Let me know 🙂

cc @SNiemann15 @vikram-redhat

@bobfuru
Copy link
Contributor

bobfuru commented Oct 15, 2020

@bobfuru I was unsure of the fixes I had to do due to this line that you recently updated: vikram-redhat@195ef0d#diff-812300ac04e88a61895332850e1e5f42004729ad650880e5cdf0b4b57f0490f1R56. These are here in this PR: https://github.com/openshift/openshift-docs/pull/26490/files#diff-812300ac04e88a61895332850e1e5f42004729ad650880e5cdf0b4b57f0490f1R56. Could you please double check it looks ok?
@vikram-redhat I don't see where I actually updated that line, seems that it was already there? If we're both talking about line 56, this one:
rd.neednet=1 console=ttysclp0 coreos.inst.install_dev=dasda coreos.inst.image_url=ftp://
It that's the case, then it LGTM!

@ktania46
Copy link
Contributor

lgtm/

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2020
@codyhoag
Copy link
Contributor

codyhoag commented Oct 19, 2020

@bobfuru can you review ade4b5f6ed2e991e4c87bf7da1f15d518625fdac? I saw you added in 281f70f, so I suspect we should readd that for bare metal. Perhaps ifdefs are required for that?

@vikram-redhat
Copy link
Contributor Author

Thanks @codyhoag and @bobfuru for making the fixes and reviewing this. I will wait for @bobfuru to confirm before merging this.

@bobfuru
Copy link
Contributor

bobfuru commented Oct 19, 2020

Thanks, @codyhoag, for helping me understand the impact here. OK to merge, @vikram-redhat. I'll then need to re-add what got removed in modules/installation-user-infra-machines-static-network.adoc and add ifdefs for IBM Z/Power version.

@codyhoag
Copy link
Contributor

codyhoag commented Oct 19, 2020

@vikram-redhat I squashed my commit that makes this PR identical with the 4.6 branch. This should now be officially ready to merge. I'll let you do the honors 🙂.

Bob will follow-up with the changes he summarized in #26490 (comment) to get the content in order. We figured it was better to merge this PR so master and 4.6 mirror each other, and then do the follow-up fix for both at the same time.

@vikram-redhat
Copy link
Contributor Author

Okie dokie - merging!

@openshift/team-documentation FYI - only because of possible impacts. If something in the install docs doesn't make sense, blame this PR (and me). ;)

But doing this allows future updates to multi-arch docs easier and consistent.

@vikram-redhat
Copy link
Contributor Author

Not to be CPed anywhere else.

Comment on lines +56 to +57
a|Optional: Bonding multiple network interfaces to a single interface is optionally supported
using the `bond=` option. In these two examples:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed? It already prepends the sentence with Optional:.

@bobfuru
Copy link
Contributor

bobfuru commented Oct 19, 2020

Proposed fix to modules/installation-user-infra-machines-static-network.adoc file is introduced in #26559.

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

Labels

size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants