-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix Power changes from PR26184 #26559
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.
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
Conversation
|
The preview will be available shortly at: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a follow-up fix to #26512.
|
@bobfuru one question: I see the Advanced RHCOS installation reference used in the IBM Power installation docs too here. I don't know much background on the subject here, so I don't know what should and shouldn't be included 😅. Do you think the references to bare metal are out of place there? If not, I think these changes are good from my perspective. |
a346e2e to
10be672
Compare
10be672 to
33b6bc0
Compare
|
@codyhoag I don't know too much about the IBM Power docs, either. I was aware that Advanced RHCOS installation reference is used in the link you mentioned, and it doesn't seem out of place to me. Ideally, that should have been included in the module metadata for cross-referencing but it doesn't seem that that happened. Another question I wonder about is why Advanced RHCOS installation reference was included in only 1 of the 2 "Installing on IBM Power" assemblies ("Restricted network IBM Power installation" but not "Installing a cluster on IBM Power"). For bare metal install docs, we add it to child assemblies. Example here. @vikram-redhat I agree with Cody, I think we are good with these changes, but I also suspect it should be added to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that we updated the title of this module but it wasn't reflected in this reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I noted in the comments of this PR, I think this should be added to this assembly as well.
33b6bc0 to
53da2c2
Compare
|
FYI, @ktania46 |
|
/cherrypick enterprise-4.6 |
|
@bobfuru: new pull request created: #26593 DetailsIn response to this:
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. |
Mostly, this PR fixes changes that were made in
modules/installation-user-infra-machines-static-network.adocin #26184. (See my inline comments in that merged PR for more details.)IIUC, the changes that were made were cosmetic. In other words, they are not hiding any functionality to a certain set of users (i.e., Z-Power).
What was changed in that file minimizes the level of information that follows. The three bulleted items describe the three ways that a user can configure advanced networking: at the live installer boot prompt, by using specific
core.instboot options, or by passing arguments directly tocoreos-installer. The tables that follow that intro cover these three scenarios.It is my conclusion that the changes in this new PR should be acceptable without conditionals necessary, and that we should therefore be able to merge this PR and CP it to 4.6 with no additional tags added. But I might be missing the bigger picture.
@vikram-redhat or @codyhoag Do you see any issue with merging and CP'ing this to 4.6?
PREVIEW LINKS