-
Notifications
You must be signed in to change notification settings - Fork 2k
issue 4837 - chore: main.go refactor #5110
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Conversation
* 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
👷 Deploy request for nginx-kubernetes-ingress pending review.Visit the deploys page to approve it
|
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
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
Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>
Fix build failure introduced while resolving conflict following merge from main
Accept review comments regarding output parameter name. Co-authored-by: Shaun <s.odonovan@f5.com> Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>
Adopt review comments regarding golang error handling. Co-authored-by: Shaun <s.odonovan@f5.com> Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>
* 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
* Initial unit-tests
* Fix unit-test TestGetAppProtectVersionInfo()
* Add unit-test for TestCreateGlobalConfigurationValidator()
* Add unit-test for validateIngressClass()
* Add unit-test for confirmMinimumK8sVersionCriteria()
* Add unit-test for getAndValidateSecret()
An idea that may be worth considering: We have many functions in the |
Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>
* Remove superfluous logging
@jjngx , this is a good idea, I shall adopt this where possible. |
* Added test for getWatchedNamespaces()
* Added test for checkNamespaceExists() * checkNamespaceExists() was modified to return a boolean for the unit-test
* Added test for createConfigClient, processDefaultServerSecret, processWildcardSecret
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. |
Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>
Excellent, what's better than reducing complexity by removing or not adding more code 👍🏻 |
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:
Checklist
Before creating a PR, run through this checklist and mark each as complete.