Skip to content

Conversation

@abhatt-rh
Copy link
Contributor

@abhatt-rh abhatt-rh commented Nov 23, 2022

TELCODOCS-1037: This PR adds instructions for nmstate network settings.

Version(s):
OpenShift 4.11+

Issue:
https://issues.redhat.com/browse/TELCODOCS-1037

Link to docs preview:
https://53122--docspreview.netlify.app/openshift-enterprise/latest/installing/installing_bare_metal_ipi/ipi-install-expanding-the-cluster.html#preparing-the-bare-metal-node_ipi-install-expanding
(Added the instruction as callout 4 under step 4 > Static configuration of the bmh.yml)

QE review:

  • QE has approved this change.

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 23, 2022
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Nov 23, 2022

🤖 Updated build preview is available at:
https://53122--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/4415

@abhatt-rh
Copy link
Contributor Author

Hi @mrbojangles3 and @cybertron,
Request you to review the changes for the nmsate network setting.

Also, do we need to add a note/warning to the following effect:

Setting nmstate.networkConfig.interfaces.state: down might still leave the interface up and show the DHCP configuration.

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I might re-word this a bit so instead of "you want to ignore the DHCP configuration" it says something like "you want to disable an interface". When people are running into this problem it's because they want to disable an interface and they may not necessarily equate ignoring DHCP to disabling it, even though in this case they're the same thing.

Also maybe mention the fact that state must be up? I know it's in the example, but that's the kind of unintuitive part here and it wouldn't hurt to reinforce it in the comment.

Choose a reason for hiding this comment

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

I agree with the above re-wording. When I got myself into this situation it was because I wanted to disable an interface. I don't fully grok the NetworkManager interface up/down flow so I defer to @cybertron for that.

Copy link
Contributor Author

@abhatt-rh abhatt-rh Nov 30, 2022

Choose a reason for hiding this comment

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

Thank you for your comments, @cybertron and @mrbojangles3
I have revised the description. PTAL and let me know for any further changes.

Copy link
Member

@cybertron cybertron left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2022
@abhatt-rh
Copy link
Contributor Author

@pamoedom Request you to review/approve the changes from QE perspective. Thanks!

@pamoedom
Copy link

pamoedom commented Dec 1, 2022

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Dec 1, 2022
@abhatt-rh abhatt-rh changed the title [WIP] TELCODOCS-1037: Add instructions for nmstate network settings TELCODOCS-1037: Add instructions for nmstate network settings Dec 1, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2022
@abhatt-rh
Copy link
Contributor Author

/label telco
/label need-peer-review

@openshift-ci openshift-ci bot added the telco Label for all Telco PRs label Dec 1, 2022
@openshift-ci
Copy link

openshift-ci bot commented Dec 1, 2022

@abhatt-rh: The label(s) /label need-peer-review cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, downstream-change-needed, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, cnv, dev-tools, distributed-tracing, ims, jira/valid-bug, merge-review-in-progress, merge-review-needed, mtc, multi-arch, oadp, peer-review-done, peer-review-in-progress, peer-review-needed, rhacs, rhv, serverless, service-mesh, staff-eng-approved, telco. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

Details

In response to this:

/label telco
/label need-peer-review

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.

@abhatt-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 Dec 1, 2022
@abhatt-rh
Copy link
Contributor Author

/remove-label peer-review-needed
Removing the label to fix callout numbering. We now have a solution :)

@openshift-ci openshift-ci bot removed the peer-review-needed Signifies that the peer review team needs to review this PR label Dec 1, 2022
@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 1, 2022
@abhatt-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 Dec 1, 2022
@Wiharris
Copy link
Contributor

Wiharris commented Dec 1, 2022

/label peer-review-in-progress

@openshift-ci openshift-ci bot added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Dec 1, 2022
Copy link
Contributor

@Wiharris Wiharris left a comment

Choose a reason for hiding this comment

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

/LGTM

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2022
@Wiharris
Copy link
Contributor

Wiharris commented Dec 1, 2022

/remove-label peer-review-in-progress
/remove-label peer-review-needed
/label peer-review-done

@openshift-ci openshift-ci bot 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 peer-review-needed Signifies that the peer review team needs to review this PR labels Dec 1, 2022
@abhatt-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 Dec 1, 2022
@mburke5678 mburke5678 added the merge-review-in-progress Signifies that the merge review team is reviewing this PR label Dec 1, 2022
@mburke5678 mburke5678 merged commit 19739fa into openshift:main Dec 1, 2022
@mburke5678
Copy link
Contributor

/cherrypick enterprise-4.11

@openshift-cherrypick-robot

@mburke5678: new pull request created: #53389

Details

In response to this:

/cherrypick enterprise-4.11

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.

@mburke5678
Copy link
Contributor

/cherrypick enterprise-4.12

@openshift-cherrypick-robot

@mburke5678: new pull request created: #53396

Details

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.

@mburke5678 mburke5678 removed merge-review-in-progress Signifies that the merge review team is reviewing this PR merge-review-needed Signifies that the merge review team needs to review this PR labels Dec 2, 2022
@abhatt-rh abhatt-rh deleted the td-1037 branch April 11, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.11 branch/enterprise-4.12 lgtm Indicates that a PR is ready to be merged. peer-review-done Signifies that the peer review team has reviewed this PR qe-approved Signifies that QE has signed off on this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files. telco Label for all Telco PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants