Normalize options on the stub and update the normalized CR #54
Conversation
|
This change is |
|
Would you like to review this one, @pavolloffay ? |
|
sure going to have a look |
pkg/stub/handler.go
Outdated
| } | ||
|
|
||
| // normalize the deployment strategy | ||
| if strings.ToLower(o.Spec.Strategy) != "production" { |
There was a problem hiding this comment.
does this operator support deploying all-in-one with real storage?
pkg/stub/handler.go
Outdated
|
|
||
| // check for incompatible options | ||
| // if the storage is `memory`, then the only possible strategy is `all-in-one` | ||
| if strings.ToLower(o.Spec.Storage.Type) == "memory" && o.Spec.Strategy != "all-in-one" { |
There was a problem hiding this comment.
Shouldn't there be ToLower on strategy?
pkg/stub/handler.go
Outdated
| @@ -59,3 +104,20 @@ func (h *Handler) Handle(ctx context.Context, event sdk.Event) error { | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func unknownStorage(typ string) bool { | |||
| known := []string{ | |||
There was a problem hiding this comment.
map/set seems more suitable
There was a problem hiding this comment.
Map or set? Do you mean, to ensure the list does not contain duplicate entries? As this is a very small static array, I don't see much value in having a map here instead of an array, but perhaps I'm missing something.
pkg/stub/handler.go
Outdated
| o.Spec.Storage.Type = "memory" | ||
| } | ||
|
|
||
| if unknownStorage(o.Spec.Storage.Type) { |
There was a problem hiding this comment.
The previous if could be merged into this.
"The provided storage type for the Jaeger instance '%v' is ('%v') Should be one of.... Falling back to 'memory'.
There was a problem hiding this comment.
It could, but the way it is, it provides a more specific log message to the user.
There was a problem hiding this comment.
What I liked from my proposal is to write all valid options in the log.
There was a problem hiding this comment.
This?
time="2018-10-10T13:09:15+02:00" level=info msg="The provided storage type for the Jaeger instance 'TestNewControllerForProduction' is unknown ('unknown'). Falling back to 'memory'. Known options: [memory kafka elasticsearch cassandra]"
|
It's my first review in this repo. I have looked at it from the language side not feature side |
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling
force-pushed
the
53-Avoid-duplicate-messages
branch
from
e418edb
to
56028e5
Compare
Codecov Report
@@ Coverage Diff @@
## master #54 +/- ##
=======================================
Coverage 99.17% 99.17%
=======================================
Files 16 16
Lines 603 603
=======================================
Hits 598 598
Misses 5 5
Continue to review full report at Codecov.
|
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @pavolloffay and @jpkrohling)
pkg/controller/controller_test.go, line 87 at r3 (raw file):
} func TestIncompatibleStorageForProduction(t *testing.T) {
nice test!
This PR moves some of the normalization logic to the stub from the controller. Once the CR is normalized, it's stored back. The side effect is that the controller will only get valid CRs, so that messages related to the normalization will happen only once.
Before this PR:
After this PR:
Closes #53
Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de