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

bpf: rename variables with camel-case names #16476

Merged
merged 1 commit into from Jul 15, 2021

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Jun 8, 2021

We run checkpatch.pl on the commits touching the bpf/ code, and it complains when edits contain camel-case variable names -- even if they were not introduced by the commit. We could disable this report in the script we use to call checkpatch.pl, but at the same time we do want it to catch the introduction of new camel-case names.

As suggested in #16447 (comment), let's fix the existing variable names once and for all, to avoid contributors to get surprised by the reports.

The number of variables is rather low: srcID, dstID, localID, remoteID, and newEntries.

@qmonnet qmonnet added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Jun 8, 2021
@qmonnet qmonnet requested review from a team and jrfastab June 8, 2021 17:06
@qmonnet
Copy link
Member Author

qmonnet commented Jun 9, 2021

test-me-please

@qmonnet
Copy link
Member Author

qmonnet commented Jun 9, 2021

test-1.21-4.9
[Multiple failures in k8s-1.21-kernel-4.9 due to Istio failing to deploy in time and causing a BeforeAll block to fail.]

@qmonnet qmonnet force-pushed the pr/bpf-camel-case branch 2 times, most recently from a738f5d to 21122fa Compare June 14, 2021 08:46
@qmonnet
Copy link
Member Author

qmonnet commented Jun 14, 2021

test-me-please

@pchaigno pchaigno added the needs/triage This issue requires triaging to establish severity and next steps. label Jun 29, 2021
@pchaigno
Copy link
Member

pchaigno commented Jun 29, 2021

The Jenkins builds expired, so I retriggered:

test-me-please

@qmonnet
Copy link
Member Author

qmonnet commented Jun 29, 2021

Apologies, I forgot to comment on the failures -_-. There were two flakes, documented as #16399 and #16551, plus some failures on the Conformance tests. Thanks for the new trigger.

@pchaigno
Copy link
Member

test-me-please

We run checkpatch.pl on the commits touching the bpf/ code, and it
complains when edits contain camel-case variable names -- even if they
were not introduced by the commit. We could disable this report in the
script we use to call checkpatch.pl, but at the same time we _do_ want
it to catch the introduction of new camel-case names.

Let's fix the existing variable names once and for all, to avoid
contributors to get surprised by the reports.

The number of variables is rather low: srcID, dstID, localID, remoteID,
and newEntries.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
Copy link
Member Author

qmonnet commented Jul 15, 2021

Rebased because the PR has been left aside for some time.
test-me-please

@qmonnet qmonnet added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed needs/triage This issue requires triaging to establish severity and next steps. labels Jul 15, 2021
@aanm aanm merged commit 576a647 into cilium:master Jul 15, 2021
@qmonnet qmonnet deleted the pr/bpf-camel-case branch July 16, 2021 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants