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

Cleanup code #1791

Merged
Merged

Conversation

cezarygerard
Copy link
Contributor

Cleanup code:

  • get rid of one letter variables named 'L/l/I/|/!/1'
  • removed unused parameters from assert func in l4 tests
  • split l4//l7 neg endpoint calculators into separate files
  • refactor l4 neg calculator

no business logic or test logic has been changed in this commit

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 25, 2022
@k8s-ci-robot k8s-ci-robot requested review from bowei and freehan August 25, 2022 16:29
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2022
@cezarygerard cezarygerard force-pushed the never-use-l-as-variable-name branch from 826b038 to 0af506a Compare August 25, 2022 16:36
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2022
@cezarygerard
Copy link
Contributor Author

/assign panslava

Copy link
Contributor

@panslava panslava left a comment

Choose a reason for hiding this comment

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

Can I ask you to move endpoints refactoring to separate PR? Let's leave here only renaming

@@ -169,7 +169,7 @@ func TestHealthCheckFirewallDeletionWithILB(t *testing.T) {
l4NetLB := NewL4NetLB(netlbSvc, fakeGCE, meta.Regional, namer, record.NewFakeRecorder(100))

// make sure both ilb and netlb use the same l4 healtcheck instance
l4NetLB.l4HealthChecks = l4ilb.l4HealthChecks
l4NetLB.l4HealthChecks = l4ilb.healthChecks
Copy link
Contributor

Choose a reason for hiding this comment

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

if we change l4healthChecks -> healthChecks on ILB struct should we also change it on NetLB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, done

- get rid of one letter variables named 'L/l/I/|/!/1'
- removed unused parameters from assert func in l4 tests

no business logic or test logic has been changed in this commit
@cezarygerard cezarygerard force-pushed the never-use-l-as-variable-name branch from 0af506a to 28095d8 Compare August 26, 2022 09:05
- get rid of one letter variables named 'L/l/I/|/!/1'
- removed unused parameters from assert func in l4 tests
- use better field names

no business logic or test logic has been changed in this commit
@cezarygerard
Copy link
Contributor Author

@panslava I chave move the neg calculators refactoring to pr/1792

@panslava
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarygerard, panslava

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [cezarygerard,panslava]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit cfcb825 into kubernetes:master Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants