-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
optimize: local cache global variable and avoid single lines over 80 #4591
optimize: local cache global variable and avoid single lines over 80 #4591
Conversation
Welcome @membphis! |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Hi @membphis. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
Codecov Report
@@ Coverage Diff @@
## master #4591 +/- ##
=========================================
Coverage ? 58.53%
=========================================
Files ? 88
Lines ? 6699
Branches ? 0
=========================================
Hits ? 3921
Misses ? 2341
Partials ? 437 Continue to review full report at Codecov.
|
rootfs/etc/nginx/lua/util.lua
Outdated
local string_format = string.format | ||
local pairs = pairs | ||
local tonumber = tonumber | ||
local ngx = ngx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will require us to adjust some unit tests as they mock some ngx
APIs before loading the modules: https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/ingress-nginx/4591/pull-ingress-nginx-test-lua/1176327555209236481
@membphis thanks for the PR! Such PRs are always welcome. I'm thinking of adding linter to force these best practices. |
it's not easy to do these the new code should be fine under Do we have any plan to run those test case in |
@membphis we use resty to run the Lua tests. All the tests run in timer phase. We already have e2e tests for the type of test you are describing. These Lua tests are unit tests. |
c9d7b26
to
193b40b
Compare
@ElvinEfendi Thanks for your explanation, we run the test case with I'll continue to fix it. |
fa1eacb
to
1cd3b3d
Compare
@membphis you can run the test locally executing the command |
@membphis also, please squash the commits |
rootfs/etc/nginx/lua/util.lua
Outdated
local getmetatable = getmetatable | ||
local type = type | ||
local next = next | ||
-- local table = table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please delete instead of leaving commented
1cd3b3d
to
1ce68c8
Compare
thanks for this, I do not know this before :( |
@aledbf all fixed, please take a look. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElvinEfendi, membphis 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:
Approvers can indicate their approval by writing |
if this commit is fine, I'll submit more later.