Enable storage options to be filtered out based on specified storage … #94
Conversation
…type Signed-off-by: Gary Brown <[email protected]>
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
==========================================
+ Coverage 99.35% 99.36% +0.01%
==========================================
Files 18 19 +1
Lines 776 791 +15
==========================================
+ Hits 771 786 +15
Misses 5 5
Continue to review full report at Codecov.
|
Signed-off-by: Gary Brown <[email protected]>
There was a problem hiding this comment.
Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @objectiser)
pkg/apis/io/v1alpha1/options.go, line 21 at r2 (raw file):
} // Filter creates a new Options object with just the elements identified by the supplied prefix
I think it would be more appropriate to be explicit about which prefixes to remove, instead of which ones to keep. The storage package could hold the logic about removing everything except one prefix.
With this change, the example file all-in-one-with-options.yaml would break, as the log-level option would be filtered out if I understood this code correctly.
pkg/apis/io/v1alpha1/options_test.go, line 52 at r2 (raw file):
o := NewOptions(map[string]interface{}{ "es.server-urls": "http://elasticsearch.default.svc:9200", "memory.max-traces": "50000",
Can you add a test with other options as well? Right now, it looks like they would get filtered out (like log-level), but would be good to have an option not related to storage in the test, to be explicit about what's the expected behavior.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
pkg/apis/io/v1alpha1/options.go, line 21 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
I think it would be more appropriate to be explicit about which prefixes to remove, instead of which ones to keep. The storage package could hold the logic about removing everything except one prefix.
With this change, the example file
all-in-one-with-options.yamlwould break, as thelog-leveloption would be filtered out if I understood this code correctly.
As pointed out, this change affects only the options within the JaegerStorageSpec, so, it makes sense to follow this approach.
…type
Signed-off-by: Gary Brown [email protected]