Skip to content

OCPBUGS-28912 Updating creating a Whereabouts reconciler daemon #72504

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 5, 2024
Merged

OCPBUGS-28912 Updating creating a Whereabouts reconciler daemon #72504

merged 1 commit into from
Mar 5, 2024

Conversation

kquinn1204
Copy link
Contributor

@kquinn1204 kquinn1204 commented Mar 4, 2024 •
edited
Loading

[OCPBUGS-28912]: Updating the guidance for adding the additionalNetworks section.

Version(s): 4.12, 4.13, 4.14, 4.15 and main

Issue: https://issues.redhat.com/browse/OCPBUGS-28912

Link to docs preview: https://72504--ocpdocs-pr.netlify.app/openshift-enterprise/latest/networking/multiple_networks/configuring-additional-network.html#nw-multus-creating-whereabouts-reconciler-daemon-set_configuring-additional-network

QE review:

  • QE has approved this change.

Additional information:

@openshift-ci openshift-ci bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 4, 2024
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Mar 4, 2024 •
edited
Loading

🤖 Tue Mar 05 14:42:34 - Prow CI generated the docs preview:
https://72504--ocpdocs-pr.netlify.app

@kquinn1204
Copy link
Contributor Author

Hi @cgoncalves @liornoy when I was verifying #71081 i noticed the guidance in this section was not 100% accurate. Can you add a LGTM to this PR If happy with the change.

@cgoncalves
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2024
@kquinn1204
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 Mar 4, 2024
@kowen-rh
Copy link
Contributor

kowen-rh commented Mar 4, 2024

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

@openshift-ci openshift-ci bot added peer-review-in-progress Signifies that the peer review team is reviewing this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Mar 4, 2024
kowen-rh
kowen-rh reviewed Mar 4, 2024
View reviewed changes
Copy link
Contributor

@kowen-rh kowen-rh left a comment

Choose a reason for hiding this comment

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

The YAML example seems like it could potentially cause issues if directly copy and pasted to me. Could we only include the specific information that's being talked about in the procedure?

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

modules/nw-multus-creating-whereabouts-reconciler-daemon-set.adoc Outdated
+
[source,yaml]
----
apiVersion: operator.openshift.io/v1
kind: Network
metadata:
creationTimestamp: "2024-01-03T11:19:12Z"
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I wouldn't include this information in the example YAML file here. It's inviting potential user error with copy and paste in my opinion.

Is this information supposed to be written by hand or generated by a tool? I see references to a generation number, resource version, and UID, all of which are unlikely to be the same between what the user is doing and what the YAML example here displays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kowen-rh I am going to look at that again and maybe just detail the exact snippet of YAML that needs included

modules/nw-multus-creating-whereabouts-reconciler-daemon-set.adoc Outdated
@@ -51,6 +55,7 @@ spec:
}
}
type: Raw
#...
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I think it would be a good idea to include this type of comment directly before the spec: key to imply that there is additional YAML both before and after the whereabouts-shim that we're telling the user to include. Does that make sense?

@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 lgtm Indicates that a PR is ready to be merged. labels Mar 4, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Mar 5, 2024

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot 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 Mar 5, 2024
@kquinn1204
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 Mar 5, 2024
@mburke5678 mburke5678 added the merge-review-in-progress Signifies that the merge review team is reviewing this PR label Mar 5, 2024
mburke5678
mburke5678 reviewed Mar 5, 2024
View reviewed changes
modules/nw-multus-creating-whereabouts-reconciler-daemon-set.adoc
apiVersion: operator.openshift.io/v1
kind: Network
metadata:
name: cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc guidelines require this section: https://github.com/openshift/openshift-docs/blob/main/contributing_to_docs/doc_guidelines.adoc#required-fields

@mburke5678
Copy link
Contributor

@kquinn1204 I have one comment that I am hoping you can consider before we merge. Reach out directly when ready.

@openshift-ci openshift-ci bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 5, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Mar 5, 2024

@kquinn1204: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@kquinn1204
Copy link
Contributor Author

Thank you @mburke5678 that is a good recommendation hopefully OK to merge now

@mburke5678 mburke5678 added this to the Continuous Release milestone Mar 5, 2024
@mburke5678 mburke5678 merged commit f5741bc into openshift:main Mar 5, 2024
@mburke5678
Copy link
Contributor

/cherrypick enterprise-4.12

@mburke5678
Copy link
Contributor

/cherrypick enterprise-4.13

@mburke5678
Copy link
Contributor

/cherrypick enterprise-4.14

@mburke5678
Copy link
Contributor

/cherrypick enterprise-4.15

@mburke5678
Copy link
Contributor

/cherrypick enterprise-4.16

@openshift-cherrypick-robot
Copy link

@mburke5678: #72504 failed to apply on top of branch "enterprise-4.12":

Applying: OCPBUGS-28912 Updating creating a Whereabouts reconciler daemon
Using index info to reconstruct a base tree...
A	modules/nw-multus-creating-whereabouts-reconciler-daemon-set.adoc
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): modules/nw-multus-creating-whereabouts-reconciler-daemon-set.adoc deleted in HEAD and modified in OCPBUGS-28912 Updating creating a Whereabouts reconciler daemon. Version OCPBUGS-28912 Updating creating a Whereabouts reconciler daemon of modules/nw-multus-creating-whereabouts-reconciler-daemon-set.adoc left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 OCPBUGS-28912 Updating creating a Whereabouts reconciler daemon
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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.

@openshift-cherrypick-robot
Copy link

@mburke5678: new pull request created: #72625

In response to this:

/cherrypick enterprise-4.13

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.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Mar 5, 2024
Merged
@openshift-cherrypick-robot
Copy link

@mburke5678: new pull request created: #72626

In response to this:

/cherrypick enterprise-4.14

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.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Mar 5, 2024
Merged
@openshift-cherrypick-robot
Copy link

@mburke5678: new pull request created: #72627

In response to this:

/cherrypick enterprise-4.15

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.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Mar 5, 2024
Merged
@openshift-cherrypick-robot
Copy link

@mburke5678: new pull request created: #72628

In response to this:

/cherrypick enterprise-4.16

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.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Mar 5, 2024
Merged
@mburke5678
Copy link
Contributor

The affected file is not in enterprise-4.12.

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

@mburke5678 mburke5678 mburke5678 left review comments

@kowen-rh kowen-rh kowen-rh left review comments

Assignees

@cgoncalves cgoncalves

Labels
branch/enterprise-4.13 branch/enterprise-4.14 branch/enterprise-4.15 branch/enterprise-4.16 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 peer-review-done Signifies that the peer review team has reviewed this PR size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
Continuous Release
Development

Successfully merging this pull request may close these issues.
6 participants
@kquinn1204 @ocpdocs-previewbot @cgoncalves @kowen-rh @mburke5678 @openshift-cherrypick-robot