Skip to content

Conversation

rastislavs
Copy link
Contributor

@rastislavs rastislavs commented Nov 20, 2023

This PR aims to re-organize BGP ConfigReconcilers to achieve a better structure and avoid conflicts in case of concurrent changes.

The most significant change is moving the code from a huge single source file (pkg/bgpv1/manager/reconcile.go + co-located reconcile_test.go) to multiple files, one source + test file per each reconciler. The changes are only structural, there is no functional change. For more details, please see commit messages in the individual commits.

@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 Nov 20, 2023
@rastislavs rastislavs added the release-note/misc This PR makes changes that have no direct user impact. label Nov 20, 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 Nov 20, 2023
@rastislavs rastislavs added area/bgp Impacts the Border Gateway Protocol feature. kind/cleanup This includes no functional changes. labels Nov 20, 2023
@rastislavs rastislavs force-pushed the bgp-reconcilers-cleanup branch 2 times, most recently from e299616 to 47bd5e6 Compare November 21, 2023 09:46
This is a code cleanup change for enhancing code readability
and modularity, without any functional changes.

Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
Splits recocnilers and their tests from a single source file to
a file per reconciler to avoid unnecessary code conflicts.

This change does not touch the logic of any reconciler, just
moves the code into newly created source files as it is.

Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
@rastislavs rastislavs force-pushed the bgp-reconcilers-cleanup branch 2 times, most recently from f6a2408 to 1e52e13 Compare November 21, 2023 10:07
@rastislavs
Copy link
Contributor Author

/test

@rastislavs rastislavs changed the title WIP: bgpv1: Reorganize config reconcilers bgpv1: Reorganize BGP config reconcilers Nov 21, 2023
@rastislavs rastislavs marked this pull request as ready for review November 21, 2023 10:59
@rastislavs rastislavs requested a review from a team as a code owner November 21, 2023 10:59
Copy link
Contributor

@harsimran-pabla harsimran-pabla left a comment

Choose a reason for hiding this comment

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

Thanks, awesome refactoring! Much needed change in reconcilers.

@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 21, 2023
@harsimran-pabla harsimran-pabla added the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Nov 21, 2023
@rastislavs rastislavs removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 21, 2023
Moves BGP Manager reconcilers into a separate package manager/reconciler
to achieve cleaner file structure and separation from the main BGP Manager logic.

Also moves DiffStore & BGPCPResourceStore into a new package manager/store,
and ServerWithConfig type into a new package manager/instance
to avoid cyclic dependencies.

Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
@rastislavs rastislavs force-pushed the bgp-reconcilers-cleanup branch from 1e52e13 to 87729e7 Compare November 21, 2023 15:26
@rastislavs rastislavs removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Nov 21, 2023
@rastislavs
Copy link
Contributor Author

/test

@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 21, 2023
@lmb lmb merged commit 9077e75 into cilium:main Nov 22, 2023
mhofstetter added a commit to mhofstetter/cilium that referenced this pull request Nov 22, 2023
This commit fixes the build caused by a merge race between
the following PRs

cilium#28474
cilium#29277

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
gandro pushed a commit that referenced this pull request Nov 22, 2023
This commit fixes the build caused by a merge race between
the following PRs

#28474
#29277

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
pjablonski123 pushed a commit to pjablonski123/cilium that referenced this pull request Dec 15, 2023
This commit fixes the build caused by a merge race between
the following PRs

cilium#28474
cilium#29277

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bgp Impacts the Border Gateway Protocol feature. kind/cleanup This includes no functional changes. 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.

3 participants