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
THREESCALE-8467 (1): Granular options provider logic #756
Conversation
/test test-e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code fix looks goods
I will approve when I test it locally
err := h.setBackendRedisOptions() | ||
if err != nil { | ||
return nil, err | ||
setOptionsFns := []func(*HighAvailabilityOptionsProvider) error{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for curiosity, it the following approach equivalent?
setOptionsFns := []func() error{}
if h.apimanager.IsExternal(appsv1alpha1.BackendRedis) {
setOptionsFns = append(setOptionsFns, h.setBackendRedisOptions)
}
if h.apimanager.IsExternal(appsv1alpha1.SystemRedis) {
setOptionsFns = append(setOptionsFns, h.setSystemRedisOptions)
}
if h.apimanager.IsExternal(appsv1alpha1.SystemDatabase) {
setOptionsFns = append(setOptionsFns, h.setSystemDatabaseOptions)
}
for _, setOptions := range setOptionsFns {
if err := setOptions(); err != nil {
return nil, err
}
}
In case they are equivalents, does you approach have any added benefit?
local test with the following APIManager fails externalComponents:
system:
database: true The operator log:
The struct validation tags should be removed to make all of them optional at
I like your approach and has a good trade-off of simplicity and valid fix. The proper fix should be removing |
The `GetHighAvailabilityOptions` method that is used by all external component reconcilers has logic that expects the secrets for all the components being external. This means that even when a dependency is configured as internal, as long as another dependency is configured external this logic will try to retrieve the secret for the internal dependency, failing the reconciliation. Fix this by making the logic only populate the options for the dependencies configured as external.
19f0e7c
to
0b3e1ed
Compare
Code Climate has analyzed commit 0b3e1ed and detected 0 issues on this pull request. View more on Code Climate. |
The
GetHighAvailabilityOptions
method that is used by all external component reconcilers has logic that expects the secrets for all the components being external. This means that even when a dependency is configured as internal, as long as another dependency is configured external this logic will try to retrieve the secret for the internal dependency, failing the reconciliation.Fix this by making the logic only populate the options for the dependencies configured as external.
Fixes https://issues.redhat.com/browse/THREESCALE-8467