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.

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

bgpv1: Use specific log message and remove unused parameter #28895

Conversation

hargrovee
Copy link

This commit doesn't introduce any functional changes; it's solely about modifications related to log messages and parameters. Specifically:

  1. In the diff(), when calling registerOrReconcileDiff and withdrawDiff, more specific log messages can be used.
  2. In the withdrawDiff(), the policy parameter is not used and can be safely removed.
  3. Additionally, withdrawDiff() is actually populating the withdraw field of reconcileDiff, while reconcileDiff doesn't have a remove field.

@hargrovee hargrovee requested a review from a team as a code owner October 31, 2023 09:36
@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 Oct 31, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 31, 2023
@rastislavs rastislavs added the release-note/misc This PR makes changes that have no direct user impact. label Oct 31, 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 Oct 31, 2023
@rastislavs rastislavs added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. area/bgp labels Oct 31, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 31, 2023
@rastislavs rastislavs added kind/cleanup This includes no functional changes. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 31, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 31, 2023
@rastislavs
Copy link
Contributor

/test

Copy link
Contributor

@rastislavs rastislavs left a comment

Choose a reason for hiding this comment

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

LGTM

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 31, 2023
@hargrovee hargrovee force-pushed the use-more-specific-log-message-and-remove-unused-parameter branch from 22fdd04 to 4a80fa4 Compare November 1, 2023 00:50
@aanm
Copy link
Member

aanm commented Nov 1, 2023

/test

@aanm aanm removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 1, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 1, 2023
@hargrovee hargrovee changed the title pkg/bgpv1: Use specific log message and remove unused parameter bgpv1: Use specific log message and remove unused parameter Nov 3, 2023
@hargrovee hargrovee force-pushed the use-more-specific-log-message-and-remove-unused-parameter branch 5 times, most recently from f71f4ca to a691162 Compare November 3, 2023 06:40
@rastislavs
Copy link
Contributor

@hargrovee please do not force-push if you are not making any further changes, it does not help to move this forward very much, as we need to re-run the tests again after each force-push.

@rastislavs
Copy link
Contributor

/test

@aanm aanm removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 3, 2023
@hargrovee
Copy link
Author

hargrovee commented Nov 3, 2023

@rastislavs Okay, I thought some failed checks might be resolved through a rebase. Thank you for the reminder.

@rastislavs
Copy link
Contributor

rastislavs commented Nov 3, 2023

@hargrovee

Okay, I thought some failed checks might be resolved through a rebase. Thank you for the reminder.

yeah, sometimes it may help, but looking at the failures it is not the case here. It seems that test have issues when connecting to external targets (1.1.1.1), which seems to be unreliable today, so maybe we should just give it some time and retry later.

@hargrovee
Copy link
Author

@hargrovee

Okay, I thought some failed checks might be resolved through a rebase. Thank you for the reminder.

yeah, sometimes it may help, but looking at the failures it is not the case here. It seems that test have issues when connecting to external targets (1.1.1.1), which seems to be unreliable today, so maybe we should just give it some time and retry later.

Okay, got it. Thanks!

This commit doesn't introduce any functional changes; it's solely about
modifications related to log messages and parameters. Specifically:
1. In the diff(), when calling registerOrReconcileDiff and withdrawDiff,
   more specific log messages can be used.
2. In the withdrawDiff(), the `policy` parameter is not used and can
   be safely removed.
3. Additionally, withdrawDiff() is actually populating the `withdraw` field
   of reconcileDiff, while reconcileDiff doesn't have a `remove` field.

Signed-off-by: Huagong Wang <wanghuagong@kylinos.cn>
@hargrovee hargrovee force-pushed the use-more-specific-log-message-and-remove-unused-parameter branch from a691162 to 39e10c4 Compare November 4, 2023 05:19
@rastislavs
Copy link
Contributor

/test

@rastislavs rastislavs added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 6, 2023
@aditighag aditighag merged commit ab56f67 into cilium:main Nov 6, 2023
61 checks passed
@hargrovee hargrovee deleted the use-more-specific-log-message-and-remove-unused-parameter branch November 7, 2023 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bgp kind/cleanup This includes no functional changes. kind/community-contribution This was a contribution made by a community member. 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants