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

fix(config): correctly check types.NamespacedName flags are set #3602

Merged
merged 6 commits into from
Feb 27, 2023

Conversation

czeslavo
Copy link
Contributor

@czeslavo czeslavo commented Feb 24, 2023

What this PR does / why we need it:

Makes every types.NamespacedName flag in the manager.Config a mo.Option which directly express the fact that these flags are optional. mo.Option have handy methods that allow checking whether a value was set or not (IsPresent() bool, IsAbsent() bool or Get() (value T, ok bool)).

We had a few cases in which we were checking that types.NamespacedName is not empty using its String() method which is implemented like so:

func (n NamespacedName) String() string {
	return n.Namespace + string(Separator) + n.Name
}

Even with no Namespace and Name set, String() != "" checks always evaluated to true due to the fixed separator.

Which issue this PR fixes:

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@czeslavo czeslavo self-assigned this Feb 24, 2023
@czeslavo czeslavo added the bug Something isn't working label Feb 24, 2023
@czeslavo czeslavo added this to the KIC v2.9.0 milestone Feb 24, 2023
@czeslavo czeslavo marked this pull request as ready for review February 24, 2023 17:57
@czeslavo czeslavo requested a review from a team as a code owner February 24, 2023 17:57
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Base: 72.6% // Head: 72.7% // Increases project coverage by +0.0% 🎉

Coverage data is based on head (5c46810) compared to base (9c97200).
Patch coverage: 74.5% of modified lines in pull request are covered.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #3602   +/-   ##
=====================================
  Coverage   72.6%   72.7%           
=====================================
  Files        131     131           
  Lines      15675   15694   +19     
=====================================
+ Hits       11385   11411   +26     
+ Misses      3535    3529    -6     
+ Partials     755     754    -1     
Impacted Files Coverage Δ
internal/manager/run.go 37.9% <0.0%> (ø)
internal/manager/setup.go 41.2% <23.0%> (-0.8%) ⬇️
internal/manager/config.go 94.4% <100.0%> (ø)
internal/manager/config_validation.go 88.3% <100.0%> (+0.9%) ⬆️
internal/manager/controllerdef.go 98.8% <100.0%> (ø)
internal/manager/validated.go 100.0% <100.0%> (+13.0%) ⬆️
...nternal/controllers/gateway/tlsroute_controller.go 71.4% <0.0%> (-2.1%) ⬇️
...ternal/controllers/gateway/grpcroute_controller.go 69.7% <0.0%> (ø)
internal/dataplane/parser/parser.go 90.7% <0.0%> (+1.6%) ⬆️
...nternal/controllers/gateway/tcproute_controller.go 72.0% <0.0%> (+2.0%) ⬆️
... and 1 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.

Copy link
Contributor

@randmonkey randmonkey left a comment

Choose a reason for hiding this comment

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

overall LGTM, some minor comments.

internal/manager/config_validation.go Outdated Show resolved Hide resolved
internal/manager/config_validation_test.go Show resolved Hide resolved
internal/manager/setup.go Outdated Show resolved Hide resolved
internal/manager/setup.go Outdated Show resolved Hide resolved
internal/manager/config.go Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 27, 2023
@czeslavo czeslavo force-pushed the fix-namespacedname-isset branch 3 times, most recently from 10939dd to 17daf2a Compare February 27, 2023 14:06
@czeslavo
Copy link
Contributor Author

@randmonkey @pmalek PTAL again, I believe all of your suggestions have been addressed: 3dec228.

pmalek
pmalek previously approved these changes Feb 27, 2023
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

I believe this should do it. Just one nit is that we might consider adding a type alias for mo.Option[types.NamespacedName] (this can be done in a separate PR though).

@czeslavo
Copy link
Contributor Author

czeslavo commented Feb 27, 2023

I believe this should do it. Just one nit is that we might consider adding a type alias for mo.Option[types.NamespacedName] (this can be done in a separate PR though).

Let's do it straight away: 5c46810

@czeslavo czeslavo requested a review from pmalek February 27, 2023 19:09
@rainest rainest enabled auto-merge (squash) February 27, 2023 22:11
@rainest rainest merged commit bcf0671 into main Feb 27, 2023
@rainest rainest deleted the fix-namespacedname-isset branch February 27, 2023 22:11
@pmalek pmalek added the fix label Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants