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

Refactor main.go #2763

Merged
merged 8 commits into from Jun 30, 2022
Merged

Refactor main.go #2763

merged 8 commits into from Jun 30, 2022

Conversation

ciarams87
Copy link
Member

@ciarams87 ciarams87 commented Jun 16, 2022

Proposed changes

Initial refactor of main.go. This pass doesn't remove any code or really change any of the functionality, just splits logic into functions for make it easier to read and understand. It also removes all the flag parsing logic to a separate flags.go file.

It also removes the getCommitInfo test as it wasn't working as expected anyway (it should have broken with the recent change to using the go binary to get the commit info but it wasn't getting the correct string)

The complexity before and after is:

Current:

121 main main cmd/nginx-ingress/main.go:204:1
8 main handleTerminationWithAppProtect cmd/nginx-ingress/main.go:916:1
6 main getBuildInfo cmd/nginx-ingress/main.go:961:1
5 main handleTermination cmd/nginx-ingress/main.go:770:1
4 main validateLocation cmd/nginx-ingress/main.go:905:1
4 main getAndValidateSecret cmd/nginx-ingress/main.go:882:1
4 main validateCIDRorIP cmd/nginx-ingress/main.go:866:1
3 main parseNginxStatusAllowCIDRs cmd/nginx-ingress/main.go:851:1
3 main validatePort cmd/nginx-ingress/main.go:825:1
3 main createGlobalConfigurationValidator cmd/nginx-ingress/main.go:754:1
2 main ready cmd/nginx-ingress/main.go:950:1
2 main validateAppProtectLogLevel cmd/nginx-ingress/main.go:835:1
2 main validateResourceName cmd/nginx-ingress/main.go:816:1
1 main getSocketClient cmd/nginx-ingress/main.go:805:1


New:

main.go

10 main processConfigMaps cmd/nginx-ingress/main.go:646:1
9 main createPlusAndLatencyCollectors cmd/nginx-ingress/main.go:574:1
8 main handleTerminationWithAppProtect cmd/nginx-ingress/main.go:490:1
8 main createTemplateExecutors cmd/nginx-ingress/main.go:271:1
8 main createCustomClients cmd/nginx-ingress/main.go:231:1
6 main createManagerAndControllerCollectors cmd/nginx-ingress/main.go:535:1
6 main main cmd/nginx-ingress/main.go:40:1
5 main handleTermination cmd/nginx-ingress/main.go:428:1
5 main processDefaultServerSecret cmd/nginx-ingress/main.go:354:1
5 main getNginxVersionInfo cmd/nginx-ingress/main.go:321:1
5 main createConfigAndKubeClient cmd/nginx-ingress/main.go:175:1
4 main processGlobalConfiguration cmd/nginx-ingress/main.go:633:1
4 main getAndValidateSecret cmd/nginx-ingress/main.go:474:1
4 main processNginxConfig cmd/nginx-ingress/main.go:408:1
4 main createPlusClient cmd/nginx-ingress/main.go:256:1
4 main kubernetesVersionInfo cmd/nginx-ingress/main.go:203:1
3 main createGlobalConfigurationValidator cmd/nginx-ingress/main.go:392:1
3 main processWildcardSecret cmd/nginx-ingress/main.go:379:1
3 main startApAgentsAndPlugins cmd/nginx-ingress/main.go:333:1
3 main validateIngressClass cmd/nginx-ingress/main.go:220:1
2 main ready cmd/nginx-ingress/main.go:524:1
2 main createNginxManager cmd/nginx-ingress/main.go:309:1
1 main getSocketClient cmd/nginx-ingress/main.go:463:1

flags.go

34 main parseFlags cmd/nginx-ingress/flags.go:179:1
11 main validationChecks cmd/nginx-ingress/flags.go:270:1
7 main getBuildInfo cmd/nginx-ingress/flags.go:395:1
5 main initialChecks cmd/nginx-ingress/flags.go:250:1
4 main validateLocation cmd/nginx-ingress/flags.go:384:1
4 main validateCIDRorIP cmd/nginx-ingress/flags.go:362:1
3 main parseNginxStatusAllowCIDRs cmd/nginx-ingress/flags.go:347:1
3 main validatePort cmd/nginx-ingress/flags.go:321:1
2 main validateAppProtectLogLevel cmd/nginx-ingress/flags.go:331:1
2 main validateResourceName cmd/nginx-ingress/flags.go:312:1

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@ciarams87 ciarams87 force-pushed the move-out-flags branch 3 times, most recently from 6ab9d65 to 9ac347a Compare June 17, 2022 14:58
@ciarams87 ciarams87 marked this pull request as ready for review June 17, 2022 15:11
Copy link
Contributor

@jjngx jjngx left a comment

Choose a reason for hiding this comment

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

👍🏻

@ciarams87 ciarams87 added the chore Pull requests for routine tasks label Jun 20, 2022
Copy link
Contributor

@shaun-nx shaun-nx left a comment

Choose a reason for hiding this comment

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

Really satisfying to see all that code taken out of main 👍

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2022

Codecov Report

Merging #2763 (a4289de) into main (816747c) will increase coverage by 0.08%.
The diff coverage is 9.52%.

❗ Current head a4289de differs from pull request most recent head e547b78. Consider uploading reports for the commit e547b78 to get more accurate results

@@            Coverage Diff             @@
##             main    #2763      +/-   ##
==========================================
+ Coverage   53.66%   53.74%   +0.08%     
==========================================
  Files          52       55       +3     
  Lines       14862    15085     +223     
==========================================
+ Hits         7975     8108     +133     
- Misses       6621     6707      +86     
- Partials      266      270       +4     
Impacted Files Coverage Δ
cmd/nginx-ingress/main.go 0.00% <0.00%> (-6.63%) ⬇️
cmd/nginx-ingress/utils.go 0.00% <0.00%> (ø)
cmd/nginx-ingress/flags.go 27.45% <27.45%> (ø)
internal/k8s/validation.go 97.79% <0.00%> (-1.43%) ⬇️
internal/k8s/configuration.go 95.39% <0.00%> (-0.09%) ⬇️
internal/k8s/controller.go 11.36% <0.00%> (-0.01%) ⬇️
pkg/apis/externaldns/validation/externaldns.go 95.89% <0.00%> (ø)
internal/k8s/utils.go 14.28% <0.00%> (+2.52%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ciarams87 ciarams87 force-pushed the move-out-flags branch 2 times, most recently from e07ec92 to e547b78 Compare June 23, 2022 21:14
@lucacome lucacome added this to the 2.3.0-k8s-ingress-controller milestone Jun 30, 2022
cmd/nginx-ingress/flags.go Outdated Show resolved Hide resolved
@ciarams87 ciarams87 merged commit 5f0de64 into main Jun 30, 2022
@ciarams87 ciarams87 deleted the move-out-flags branch June 30, 2022 11:06
ciarams87 added a commit that referenced this pull request Jun 30, 2022
* Create separate flag file

* Move our Prometheus logic into separate functions

* Move out gc and sm logic into separate functions

* Move most logic from main.go to separate functions

* split parseFlags a bit

* Remove redundant test

* Move getBuildInfo to utils.go

* Move if statement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants