Skip to content
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

Documentation: include bgp cli commands in bgp-cp documentation #25691

Merged
merged 1 commit into from Jun 16, 2023
Merged

Documentation: include bgp cli commands in bgp-cp documentation #25691

merged 1 commit into from Jun 16, 2023

Conversation

harsimran-pabla
Copy link
Contributor

Added CLI section in BGP control plane document.

@harsimran-pabla harsimran-pabla requested review from a team as code owners May 25, 2023 21:05
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 25, 2023
@harsimran-pabla harsimran-pabla added area/bgp area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. release-note/misc This PR makes changes that have no direct user impact. and removed area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels May 25, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 25, 2023
zacharysarah
zacharysarah requested changes May 25, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@harsimran-pabla Thanks for these updates. There are some things that need fixing, otherwise LGTM.

Documentation/network/bgp-control-plane.rst Outdated Show resolved Hide resolved
Documentation/network/bgp-control-plane.rst Outdated Show resolved Hide resolved
Documentation/network/bgp-control-plane.rst Show resolved Hide resolved
Documentation/network/bgp-control-plane.rst Outdated Show resolved Hide resolved
Documentation/network/bgp-control-plane.rst Show resolved Hide resolved
@harsimran-pabla
Copy link
Contributor Author

harsimran-pabla commented May 26, 2023 •
edited

Thanks @zacharysarah for the review! English is hard :) I have updated the document, however some of the changes which you requested are part of CLI output, which will require changes to CLI code.

I will circle back to it at later point.

@harsimran-pabla
Copy link
Contributor Author

Hi @zacharysarah whenever you get sometime, please have another look at the changes.

zacharysarah
zacharysarah approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@harsimran-pabla Thanks for addressing review feedback. As noted in review comments, don't rely on the "sorry, it's upstream" justification to reject changes in future PRs. Pressure to fix bad docs has to start somewhere.

@harsimran-pabla
Copy link
Contributor Author

Totally agree with you @zacharysarah, I will fix the CLI code shortly.

@harsimran-pabla
Documentation: include bgp cli commands in bgp-cp documentation
be9bfff 
Added CLI section in bgp control plane document.

Signed-off-by: harsimran pabla <hpabla@isovalent.com>
@harsimran-pabla harsimran-pabla mentioned this pull request Jun 16, 2023
Merged
@harsimran-pabla
Copy link
Contributor Author

harsimran-pabla commented Jun 16, 2023 •
edited

Doc tests and review done, marking ready-to-merge.
(Documentation only change, skipping full test suite)

@harsimran-pabla harsimran-pabla added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 16, 2023
@borkmann borkmann merged commit 627f518 into cilium:main Jun 16, 2023
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zacharysarah zacharysarah zacharysarah approved these changes

@ldelossa ldelossa ldelossa approved these changes

Assignees
No one assigned
Labels
area/bgp ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@harsimran-pabla @zacharysarah @ldelossa @borkmann