issue 4837 - chore: main.go refactor #5110

mrajagopal · 2024-02-16T04:59:04Z

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

netlify · 2024-02-16T04:59:07Z

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

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`513a8b1`

cmd/nginx-ingress/main.go

jjngx · 2024-02-16T14:36:32Z

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

mrajagopal · 2024-02-18T10:09:58Z

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 where appropriate

Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>

Fix build failure introduced while resolving conflict following merge from main

cmd/nginx-ingress/main.go

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()

* Fix unit-test TestGetAppProtectVersionInfo()

* Add unit-test for getAndValidateSecret()

jjngx · 2024-03-12T16:53:26Z

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.

Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>

* Remove superfluous logging

mrajagopal · 2024-04-23T02:53:53Z

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.

* 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

mrajagopal · 2024-05-02T04:33:50Z

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 added 4 commits February 2, 2024 11:58

Issue 4837: chore: Rename main_test.go to flags_test.go

6bb3949

* All the tests contained were for flags.go functions anyway * Therefore, it makes sense to rename it as such.

Merge branch 'nginxinc:main' into mrajagopal-issue-4837

0f05611

Merge branch 'nginxinc:main' into mrajagopal-issue-4837

beddad0

Issue 4837: chore: main.go refactor

39aa5f0

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

j1m-ryan reviewed Feb 16, 2024

View reviewed changes

cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved

mrajagopal added 7 commits February 19, 2024 15:50

Issue 4837: chore: main.go refactor

f2b2873

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

Merge branch 'nginxinc:main' into mrajagopal-issue-4837

b95c264

Issue 4837: chore: main.go refactor

fb29d9a

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

Issue 4837: chore: main.go refactor

4de9b37

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

Merge branch 'main' into mrajagopal-issue-4837

e139372

Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>

Issue 4837: chore: main.go refactor

8ae17f9

Fix build failure introduced while resolving conflict following merge from main

chore: This is pre-commit install

7b88141

shaun-nx reviewed Feb 26, 2024

View reviewed changes

cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved

shaun-nx reviewed Feb 26, 2024

View reviewed changes

cmd/nginx-ingress/main.go Outdated Show resolved Hide resolved

mrajagopal and others added 13 commits February 27, 2024 09:28

Update cmd/nginx-ingress/main.go

8a0f5d8

Accept review comments regarding output parameter name. Co-authored-by: Shaun <s.odonovan@f5.com> Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>

Update cmd/nginx-ingress/main.go

2162fd7

Adopt review comments regarding golang error handling. Co-authored-by: Shaun <s.odonovan@f5.com> Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>

Issue 4837: chore: main.go refactor

4b456e6

* Fix build failure introduced after merging reviewed code changes * Address linter errors highlighted as part of pre-commit hook checks

Issue 4837: chore: main.go refactor

668c7d3

* Reduce the scope of the error returned as the error that is used here is never re-used outside the scope of the return

Issue 4837: chore: main.go refactor

e9e37e9

* Add function header info where appropriate

Issue 4837: chore: main.go refactor

a60f7e2

Issue 4837: chore: main.go refactor

05736e3

* Added initial unit-tests for some functions

Issue 4837: chore: main.go refactor

88cfd23

* Revert unit-tests to determine golangci-lint pre-commit errors

Issue 4837: chore: main_test.go unit-tests

fa3dfc9

* Initial unit-tests

Issue 4837: chore: main_test.go unit-tests

eb72565

* Fix unit-test TestGetAppProtectVersionInfo()

Issue 4837: chore: main_test.go unit-tests

d37e3bb

* Add unit-test for TestCreateGlobalConfigurationValidator()

Issue 4837: chore: main_test.go unit-tests

9b17488

* Add unit-test for validateIngressClass()

Issue 4837: chore: main_test.go unit-tests

86e7522

* Add unit-test for confirmMinimumK8sVersionCriteria()

mrajagopal added 3 commits March 5, 2024 21:34

Issue 4837: chore: main_test.go unit-tests

ffd5e52

* Fix unit-test TestGetAppProtectVersionInfo()

Issue 4837: chore: main_test.go unit-tests

2dec987

* Add unit-test for getAndValidateSecret()

Merge branch 'main' into mrajagopal-issue-4837

27d3e01

Merge branch 'main' into mrajagopal-issue-4837

e9e8a2b

Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com>

github-actions bot added the go Pull requests that update Go code label Apr 22, 2024

Issue 4837: chore: main_test.go unit-tests

d3bf5ac

* Remove superfluous logging

mrajagopal and others added 11 commits April 24, 2024 14:16

Issue 4837: chore: main_test.go unit-tests

5d26b05

* Added test for getWatchedNamespaces()

Issue 4837: chore: main_test.go unit-tests

b6aacea

* Added test for checkNamespaceExists() * checkNamespaceExists() was modified to return a boolean for the unit-test

Merge branch 'main' into mrajagopal-issue-4837

829cf3b

Merge branch 'main' into mrajagopal-issue-4837

2e3b85a

Merge branch 'main' into mrajagopal-issue-4837

345fff2

Merge branch 'main' into mrajagopal-issue-4837

d848e6f

Merge branch 'main' into mrajagopal-issue-4837

ef3e788

Merge branch 'main' into mrajagopal-issue-4837

9d56606

Merge branch 'main' into mrajagopal-issue-4837

1ee9d4b

Merge branch 'main' into mrajagopal-issue-4837

13cd17b

Issue 4837: chore: main_test.go unit-tests

8567a45

* Added test for createConfigClient, processDefaultServerSecret, processWildcardSecret

mrajagopal marked this pull request as ready for review May 2, 2024 04:36

mrajagopal requested a review from a team as a code owner May 2, 2024 04:36

mrajagopal changed the title ~~Draft: issue 4837 - chore: main.go refactor~~ issue 4837 - chore: main.go refactor May 2, 2024

Merge branch 'main' into mrajagopal-issue-4837

513a8b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue 4837 - chore: main.go refactor #5110

issue 4837 - chore: main.go refactor #5110

mrajagopal commented Feb 16, 2024 •

edited

netlify bot commented Feb 16, 2024 •

edited

jjngx commented Feb 16, 2024

mrajagopal commented Feb 18, 2024 •

edited

jjngx commented Mar 12, 2024

mrajagopal commented Apr 23, 2024 •

edited

mrajagopal commented May 2, 2024

issue 4837 - chore: main.go refactor #5110

Are you sure you want to change the base?

issue 4837 - chore: main.go refactor #5110

Conversation

mrajagopal commented Feb 16, 2024 • edited

Proposed changes

Checklist

netlify bot commented Feb 16, 2024 • edited

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

jjngx commented Feb 16, 2024

mrajagopal commented Feb 18, 2024 • edited

jjngx commented Mar 12, 2024

mrajagopal commented Apr 23, 2024 • edited

mrajagopal commented May 2, 2024

mrajagopal commented Feb 16, 2024 •

edited

netlify bot commented Feb 16, 2024 •

edited

mrajagopal commented Feb 18, 2024 •

edited

mrajagopal commented Apr 23, 2024 •

edited