Add support repetitive arguments to operand #1434

rubenvp8510 · 2021-04-21T02:57:17Z

Signed-off-by: Ruben Vargas [email protected]

jpkrohling · 2021-04-22T10:01:51Z

CI seems to be failing.

# github.com/jaegertracing/jaeger-operator/pkg/service
pkg/service/collector.go:111:36: cannot use val (type interface {}) as type string in argument to strconv.ParseBool: need type assertion
# github.com/jaegertracing/jaeger-operator/pkg/cronjob
pkg/cronjob/es_index_cleaner.go:22:62: cannot use jaeger.Spec.Storage.Options.Map() (type map[string]interface {}) as type map[string]string in argument to util.GetEsHostname
pkg/cronjob/es_index_cleaner.go:31:92: cannot use val (type interface {}) as type string in argument to strings.EqualFold: need type assertion
pkg/cronjob/es_rollover.go:92:87: cannot use jaeger.Spec.Storage.Options.Map() (type map[string]interface {}) as type map[string]string in argument to util.GetEsHostname
pkg/cronjob/es_rollover.go:161:54: cannot use val (type interface {}) as type string in field value: need type assertion
pkg/cronjob/spark_dependencies.go:129:39: cannot use sFlagsMap["cassandra.servers"] (type interface {}) as type string in field value: need type assertion
pkg/cronjob/spark_dependencies.go:130:33: cannot use keyspace (type interface {}) as type string in field value: need type assertion
pkg/cronjob/spark_dependencies.go:131:33: cannot use sFlagsMap["cassandra.username"] (type interface {}) as type string in field value: need type assertion
pkg/cronjob/spark_dependencies.go:132:33: cannot use sFlagsMap["cassandra.password"] (type interface {}) as type string in field value: need type assertion
pkg/cronjob/spark_dependencies.go:133:32: cannot use sFlagsMap["cassandra.tls"] (type interface {}) as type string in field value: need type assertion
pkg/cronjob/spark_dependencies.go:134:33: cannot use sFlagsMap["cassandra.local-dc"] (type interface {}) as type string in field value: need type assertion
pkg/cronjob/spark_dependencies.go:134:33: too many errors
make[1]: *** [Makefile:80: gobuild] Error 2
make[1]: Leaving directory '/home/runner/work/jaeger-operator/jaeger-operator'
Failed to build the operator.

codecov · 2021-04-27T06:46:12Z

Codecov Report

Merging #1434 (014d50c) into master (05b0c69) will increase coverage by 0.02%.
The diff coverage is 96.36%.

@@            Coverage Diff             @@
##           master    #1434      +/-   ##
==========================================
+ Coverage   87.70%   87.73%   +0.02%     
==========================================
  Files          93       93              
  Lines        5839     5868      +29     
==========================================
+ Hits         5121     5148      +27     
- Misses        552      553       +1     
- Partials      166      167       +1

Impacted Files	Coverage Δ
pkg/util/util.go	`93.93% <60.00%> (-1.13%)`	⬇️
pkg/apis/jaegertracing/v1/options.go	`92.68% <100.00%> (+3.02%)`	⬆️
pkg/cronjob/es_index_cleaner.go	`100.00% <100.00%> (ø)`
pkg/cronjob/es_rollover.go	`95.93% <100.00%> (ø)`
pkg/cronjob/spark_dependencies.go	`92.59% <100.00%> (ø)`
pkg/ingress/query.go	`100.00% <100.00%> (ø)`
pkg/service/collector.go	`100.00% <100.00%> (ø)`
pkg/storage/cassandra_dependencies.go	`100.00% <100.00%> (ø)`
pkg/storage/elasticsearch_dependencies.go	`100.00% <100.00%> (ø)`
pkg/strategy/controller.go	`94.32% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05b0c69...014d50c. Read the comment docs.

jpkrohling · 2021-05-05T08:20:31Z

Let me know when this gets ready to be reviewed.

rubenvp8510 · 2021-05-06T14:24:04Z

This is ready for review

jpkrohling · 2021-05-11T11:41:37Z

pkg/apis/jaegertracing/v1/options_test.go

@@ -5,6 +5,8 @@ import (
 	"sort"
 	"testing"

+	"github.com/stretchr/testify/require"
+


Import order.

Looks like this hasn't been fixed.

jpkrohling · 2021-05-11T11:44:12Z

pkg/apis/jaegertracing/v1/options_test.go

+
+func TestStringMap(t *testing.T) {
+	o := NewOptions(nil)
+	err := o.UnmarshalJSON([]byte(`{"firstsarg":"v1", "additional-headers":["whatever:thing", "access-control-allow-origin:blerg"]}`))


Can you add an example with real values here? I'm failing to see why additional-headers should be removed instead of having something like --firstsarg=v1 --additional-headers="whatever:thing" --additional-headers="access-control-allow-origin:blerg"

I'll add some real examples,

This is testing a new method I introduced in this PR. StringMap, this method only returns a map with strings (mimic the old behavior when we didn't support arrays) , it omits repeated arguments, I did in this way because this map is used in a lot of places, and I wand to preserve that behaviour for those places which is used, also I don't want to do type asserting each time the option map is used.

jpkrohling · 2021-06-03T07:18:49Z

pkg/apis/jaegertracing/v1/options.go

@@ -7,10 +7,28 @@ import (
 	"strings"
 )

+// Values represent and string or a slice of string


This doesn't seem accurate. Perhaps you meant "Values hold a map, with string as the key and either a string or a slice of strings as the value"

In any case, the name for this struct isn't really telling what the struct is. Can you come up with a better name?

Better comment :) I'll change it, and rename this to a better name

jpkrohling · 2021-06-03T07:19:29Z

pkg/apis/jaegertracing/v1/options.go

@@ -69,18 +86,24 @@ func (o Options) MarshalJSON() ([]byte, error) {
 }

 func (o *Options) parse(entries map[string]interface{}) {
-	o.opts = make(map[string]string)
+	o.opts = make(map[string]interface{})


Don't we have a structure with this signature already?

I can't find it, We have FreeForm but that is a different structure with json *[]byte

jpkrohling · 2021-06-03T07:20:02Z

pkg/apis/jaegertracing/v1/options_test.go

@@ -5,6 +5,8 @@ import (
 	"sort"
 	"testing"

+	"github.com/stretchr/testify/require"
+


Looks like this hasn't been fixed.

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 · 2021-07-30T05:18:47Z

@pavolloffay Could you please review?

Thank you!

pavolloffay

LGTM, one test job is failing

rubenvp8510 force-pushed the multi-options branch 2 times, most recently from 64367dd to 7ceebc2 Compare April 21, 2021 04:17

rubenvp8510 requested a review from jpkrohling May 6, 2021 14:24

jpkrohling reviewed May 11, 2021

View reviewed changes

rubenvp8510 requested a review from jpkrohling June 1, 2021 02:56

jpkrohling reviewed Jun 3, 2021

View reviewed changes

rubenvp8510 added 5 commits July 30, 2021 00:14

Add support repetitive arguments to operand

864e940

Signed-off-by: Ruben Vargas <[email protected]>

Add MapString method to get only strings from Options type

4048e37

Signed-off-by: Ruben Vargas <[email protected]>

Add options tests for stringMap and Values

a62d5e4

Signed-off-by: Ruben Vargas <[email protected]>

Fix upgrde tests

2d48777

Signed-off-by: Ruben Vargas <[email protected]>

Fix import order, improve comments for new type

eae65f7

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the multi-options branch from bc554aa to eae65f7 Compare July 30, 2021 05:18

rubenvp8510 requested a review from pavolloffay July 30, 2021 05:18

pavolloffay approved these changes Aug 3, 2021

View reviewed changes

Merge branch 'master' into multi-options

014d50c

jpkrohling enabled auto-merge (squash) August 5, 2021 13:36

jpkrohling disabled auto-merge August 5, 2021 13:36

jpkrohling enabled auto-merge (squash) August 5, 2021 13:36

jpkrohling merged commit 5ec0aa1 into jaegertracing:master Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support repetitive arguments to operand #1434

Add support repetitive arguments to operand #1434

rubenvp8510 commented Apr 21, 2021

jpkrohling commented Apr 22, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading

jpkrohling commented May 5, 2021

rubenvp8510 commented May 6, 2021

jpkrohling May 11, 2021

jpkrohling Jun 3, 2021

jpkrohling May 11, 2021

rubenvp8510 May 12, 2021

jpkrohling Jun 3, 2021

rubenvp8510 Jun 3, 2021

jpkrohling Jun 3, 2021

rubenvp8510 Jun 3, 2021

jpkrohling Jun 3, 2021

rubenvp8510 commented Jul 30, 2021

pavolloffay left a comment

Add support repetitive arguments to operand #1434

Add support repetitive arguments to operand #1434

Conversation

rubenvp8510 commented Apr 21, 2021

jpkrohling commented Apr 22, 2021 • edited Loading

codecov bot commented Apr 27, 2021 • edited Loading

Codecov Report

jpkrohling commented May 5, 2021

rubenvp8510 commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 commented Jul 30, 2021

pavolloffay left a comment

Choose a reason for hiding this comment

jpkrohling commented Apr 22, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading