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

feat: add Config.Validate() to allow validation of configuration options before starting the manager #3405

Merged
merged 3 commits into from
Jan 19, 2023

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Jan 19, 2023

What this PR does / why we need it:

This PR adds Config.Validate() to allow validation of configuration options before starting the manager.

This will also set the stage to allow stopping the manager boot up process when mutually exclusive options are being provided (e.g. as in single controller deployments kong-admin-url and potential future Admin API service name specified for service discovery).

Additionally this performs some minor refactor around config and passing down the configuration options to downstream consumers of said options. It also stores the flagset in the Config so that we can use c.flagSet.Changed() to verify if the flag has been provided by the user ( in case the default flag value is a non empty/default value).

@pmalek pmalek added area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. refactor labels Jan 19, 2023
@pmalek pmalek added this to the KIC v2.9.0 milestone Jan 19, 2023
@pmalek pmalek self-assigned this Jan 19, 2023
@pmalek pmalek marked this pull request as ready for review January 19, 2023 12:07
@pmalek pmalek requested a review from a team as a code owner January 19, 2023 12:07
@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Base: 74.0% // Head: 73.8% // Decreases project coverage by -0.3% ⚠️

Coverage data is based on head (82f45e5) compared to base (e9dd1b5).
Patch coverage: 39.5% of modified lines in pull request are covered.

❗ Current head 82f45e5 differs from pull request most recent head 1d5a344. Consider uploading reports for the commit 1d5a344 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #3405     +/-   ##
=======================================
- Coverage   74.0%   73.8%   -0.3%     
=======================================
  Files        113     114      +1     
  Lines      13892   13928     +36     
=======================================
- Hits       10291   10287      -4     
- Misses      2954    2985     +31     
- Partials     647     656      +9     
Impacted Files Coverage Δ
internal/cmd/rootcmd/run.go 21.0% <0.0%> (-4.0%) ⬇️
internal/cmd/rootcmd/rootcmd.go 13.8% <16.1%> (+13.8%) ⬆️
internal/manager/setup.go 55.6% <18.5%> (-5.1%) ⬇️
internal/manager/run.go 44.7% <26.6%> (-9.2%) ⬇️
internal/manager/flagnamespacedname.go 85.7% <85.7%> (ø)
internal/manager/config.go 90.7% <100.0%> (+0.5%) ⬆️
internal/manager/controllerdef.go 98.7% <100.0%> (ø)
internal/manager/feature_gates.go 100.0% <100.0%> (ø)
...nternal/controllers/gateway/udproute_controller.go 69.5% <0.0%> (-4.3%) ⬇️
...ternal/controllers/gateway/httproute_controller.go 58.5% <0.0%> (-1.6%) ⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pmalek pmalek changed the title feat: this adds Config.Validate() to allow validation of configuration options before starting the manager feat: add Config.Validate() to allow validation of configuration options before starting the manager Jan 19, 2023
Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

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

I like the direction of making the validation run as soon as possible, not leaving the responsibility to the routines that accept the Config. It's gonna be less error-prone and will save us from scattering validation code over places that should not be responsible for that.

internal/manager/config.go Show resolved Hide resolved
internal/manager/config.go Outdated Show resolved Hide resolved
@pmalek pmalek force-pushed the add-config-validate branch 2 times, most recently from e28c3d4 to 03a2cef Compare January 19, 2023 15:02
@pmalek pmalek enabled auto-merge (squash) January 19, 2023 15:35
@pmalek pmalek merged commit 9c2a61b into main Jan 19, 2023
@pmalek pmalek deleted the add-config-validate branch January 19, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. refactor size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants