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

issue 4837 - chore: main.go refactor #5110

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

mrajagopal
Copy link
Contributor

@mrajagopal mrajagopal commented Feb 16, 2024

Proposed changes

The objective of this PR is to give test coverage for main.go as outlined in issue 4837.
This includes improving functions by:

  • returning the error instead of throwing a fatal error
  • Split functions to improve readability and testability

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

* All the tests contained were for flags.go functions anyway
* Therefore, it makes sense to rename it as such.
The objective of this change is to make functions testable and/or easy to read
* replace fatal errors with formatted errors to be returned to the caller for error handling
* split functions were appropriate
Copy link

netlify bot commented Feb 16, 2024

👷 Deploy request for nginx-kubernetes-ingress pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2a273f8

cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
@jjngx
Copy link
Contributor

jjngx commented Feb 16, 2024

Hi @mrajagopal, I want to ask a more general question about what is the intention behind this PR?

@mrajagopal
Copy link
Contributor Author

mrajagopal commented Feb 18, 2024

Hi @mrajagopal, I want to ask a more general question about what is the intention behind this PR?

@jjngx , the objective of this PR is to give test coverage for main.go.
This particular commit attempts to improve functions by:

  • returning the error instead of throwing a fatal error
  • Split functions to improve readability and testability

I have updated the PR notes to reflect this.

The objective of this change is to make functions testable and/or easy to read
* replace fatal errors with formatted errors to be returned to the caller for error handling
* split functions were appropriate
The objective of this change is to make functions testable and/or easy to read
* replace fatal errors with formatted errors to be returned to the caller for error handling
* split functions were appropriate
The objective of this change is to make functions testable and/or easy to read
* replace fatal errors with formatted errors to be returned to the caller for error handling
* split functions where appropriate
Fix build failure introduced while resolving conflict following merge from main
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved
mrajagopal and others added 13 commits February 27, 2024 09:28
Accept review comments regarding output parameter name.

Co-authored-by: Shaun <[email protected]>
Signed-off-by: Madhu Rajagopal <[email protected]>
Adopt review comments regarding golang error handling.

Co-authored-by: Shaun <[email protected]>
Signed-off-by: Madhu Rajagopal <[email protected]>
* Fix build failure introduced after merging reviewed code changes
* Address linter errors highlighted as part of pre-commit hook checks
* Reduce the scope of the error returned as the error that is used here is never re-used outside the scope of the return
* Add function header info where appropriate
* Added initial unit-tests for some functions
* Revert unit-tests to determine golangci-lint pre-commit errors
* Fix unit-test TestGetAppProtectVersionInfo()
* Add unit-test for TestCreateGlobalConfigurationValidator()
* Add unit-test for validateIngressClass()
* Add unit-test for confirmMinimumK8sVersionCriteria()
@jjngx
Copy link
Contributor

jjngx commented Mar 12, 2024

An idea that may be worth considering:

We have many functions in the main that call log.Fatal in case of an error. It maybe worth considering to the must prefix (eg. mustParse(), mustXYZ()), check the error and call fatal inside the functions. This way we would communicate to the reader that the func either works or NIC won't start, we would have less error checking in the main.

@github-actions github-actions bot added the go Pull requests that update Go code label Apr 22, 2024
* Remove superfluous logging
@mrajagopal
Copy link
Contributor Author

mrajagopal commented Apr 23, 2024

An idea that may be worth considering:

We have many functions in the main that call log.Fatal in case of an error. It maybe worth considering to the must prefix (eg. mustParse(), mustXYZ()), check the error and call fatal inside the functions. This way we would communicate to the reader that the func either works or NIC won't start, we would have less error checking in the main.

@jjngx , this is a good idea, I shall adopt this where possible.

@mrajagopal
Copy link
Contributor Author

An idea that may be worth considering:
We have many functions in the main that call log.Fatal in case of an error. It maybe worth considering to the must prefix (eg. mustParse(), mustXYZ()), check the error and call fatal inside the functions. This way we would communicate to the reader that the func either works or NIC won't start, we would have less error checking in the main.

@jjngx , this is a good idea, I shall adopt this where possible.

Having reviewed all these functions, changing them to not return an error but log the log fatal within the function would make the unit-tests for these function redundant. Therefore, for the time being we can continue with returning an error.

@mrajagopal mrajagopal marked this pull request as ready for review May 2, 2024 04:36
@mrajagopal mrajagopal requested a review from a team as a code owner May 2, 2024 04:36
@mrajagopal mrajagopal changed the title Draft: issue 4837 - chore: main.go refactor issue 4837 - chore: main.go refactor May 2, 2024
@j1m-ryan j1m-ryan assigned j1m-ryan and shaun-nx and unassigned j1m-ryan Jul 29, 2024
@jjngx
Copy link
Contributor

jjngx commented Aug 12, 2024

An idea that may be worth considering:
We have many functions in the main that call log.Fatal in case of an error. It maybe worth considering to the must prefix (eg. mustParse(), mustXYZ()), check the error and call fatal inside the functions. This way we would communicate to the reader that the func either works or NIC won't start, we would have less error checking in the main.

@jjngx , this is a good idea, I shall adopt this where possible.

Having reviewed all these functions, changing them to not return an error but log the log fatal within the function would make the unit-tests for these function redundant. Therefore, for the time being we can continue with returning an error.

Excellent, what's better than reducing complexity by removing or not adding more code 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code
Projects
Status: Todo ☑
Development

Successfully merging this pull request may close these issues.

5 participants