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

Don't count the wrong things #116

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Conversation

cjeker
Copy link
Contributor

@cjeker cjeker commented Feb 29, 2024

The count in processData() makes little sense as a return value, one can count the various slices with len() just as easily.
Especially since it does not include the vap and bgpsec keys.

As a result reloadFromCurrentState() gets utterly confused and creates new versions when there is no change.

Fix for #114

…oes not include the vap and bgpsec keys.

As a result reloadFromCurrentState() gets utterly confused and creates new versions when there is no change.
Fix for bgp#114
@ties
Copy link
Collaborator

ties commented Feb 29, 2024

Is my read correct that the only use left for countv4/countv6 is pre-deduplication counts for metrics?

@cjeker
Copy link
Contributor Author

cjeker commented Feb 29, 2024

Yes, pretty much.

@ties
Copy link
Collaborator

ties commented Feb 29, 2024

...and the test.

I would almost remove it, since you can mistake it with some of the other ints in the result, with bad results.

@cjeker
Copy link
Contributor Author

cjeker commented Feb 29, 2024

Right now this fixes a real bug. Removing more is always possible but should be a step on top of this.

@ties
Copy link
Collaborator

ties commented Feb 29, 2024

Right now this fixes a real bug. Removing more is always possible but should be a step on top of this.

To me addressing that was well would remove the root cause of the bug as well. But this fixes a real bug as is, so lgtm

@ties ties merged commit 78a0c69 into bgp:master Feb 29, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants