[chore] make Config a struct with config fields, but no logic #3958
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iblancasa
requested changes
Apr 30, 2025
atoulme
force-pushed
the
move_logic_out_of_config
branch
3 times, most recently
from
4b82974 to
2c00d6a
Compare
…the logic out to autodetect
atoulme
force-pushed
the
move_logic_out_of_config
branch
from
2c00d6a to
8f1ac44
Compare
iblancasa
approved these changes
May 2, 2025
swiatekm
approved these changes
May 2, 2025
swiatekm
reviewed
May 2, 2025
| @@ -185,3 +187,45 @@ func (a *autoDetect) TargetAllocatorAvailability() (targetallocator.Availability | |||
| func (a *autoDetect) FIPSEnabled(_ context.Context) bool { | |||
| return fips.IsFipsEnabled() | |||
| } | |||
|
|
|||
| // ApplyAutoDetect attempts to automatically detect relevant information for this operator. | |||
| func ApplyAutoDetect(autoDetect AutoDetect, c *config.Config, logger logr.Logger) error { | |||
There was a problem hiding this comment.
Is there any reason not to make this a method on AutoDetect?
There was a problem hiding this comment.
Yes, though this is a more impactful change since we need all mocks to implement it. That might also change the way the interface works.
jaronoff97
reviewed
May 2, 2025
| @@ -185,3 +187,45 @@ func (a *autoDetect) TargetAllocatorAvailability() (targetallocator.Availability | |||
| func (a *autoDetect) FIPSEnabled(_ context.Context) bool { | |||
| return fips.IsFipsEnabled() | |||
| } | |||
|
|
|||
| // ApplyAutoDetect attempts to automatically detect relevant information for this operator. | |||
| func ApplyAutoDetect(autoDetect AutoDetect, c *config.Config, logger logr.Logger) error { | |||
| func ApplyAutoDetect(autoDetect AutoDetect, c *config.Config, logger logr.Logger) error { | ||
| logger.V(2).Info("auto-detecting the configuration based on the environment") | ||
|
|
||
| ora, err := autoDetect.OpenShiftRoutesAvailability() |
There was a problem hiding this comment.
I'm wondering if we should make this use something like the options pattern for when we instantiate the auto detect resource that we can just pass a list of options for the auto detect struct.
There was a problem hiding this comment.
I don't quite understand why we use Options, these are internal structs with no external APIs. Do we have to watch for something in particular?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By moving the logic out of Config, we make its lifecycle simple as a struct. We can then start to simplify how it is initialized and load it from a file, CLI flags, env vars and so on.