New DeploymentStrategy type for JaegerSpec.Strategy #704

volmedo · 2019-10-17T11:48:50Z

This PR adds a new type, DeploymentStrategy, to be used in JaegerSpec.Strategy instead of just a string. Constants based on the new type are also defined to define valid values.

When merged, this PR will close #137.

To do

make sure all unserialized values are converted to lower case
a test ensuring that current CRs with a mixed-case strategy are correctly parsed
the constant in the strategy package, mentioned in Make JaegerSpec.Strategy a type #137

Signed-off-by: volmedo <[email protected]>

jpkrohling

Looks really good so far! It's missing a couple of things, like:

make sure all unserialized values are converted to lower case
possibly a test ensuring that current CRs with a mixed-case strategy are correctly parsed
the constant in the strategy package, mentioned in Make JaegerSpec.Strategy a type #137

Apart from that, LGTM.

jpkrohling · 2019-10-17T13:12:29Z

pkg/deployment/collector.go

 	// to Kafka, and the storage options will be used in the Ingester instead
-	if strings.EqualFold(c.jaeger.Spec.Strategy, "streaming") {
+	if c.jaeger.Spec.Strategy == v1.DeploymentStrategyStreaming {


What would happen if we have a CR with a strategy set to Streaming?

I was assuming that the value was being normalized somewhere else. Sorry, my bad.

jpkrohling · 2019-10-17T13:13:49Z

pkg/apis/jaegertracing/v1/jaeger_types.go

@@ -49,11 +49,33 @@ const (
 	IngressSecurityOAuthProxy IngressSecurityType = "oauth-proxy"
 )

+// DeploymentStrategy represents the possible values for deployment strategies
+// +k8s:openapi-gen=true
+type DeploymentStrategy string


You could implement the UnmarshalJSON method to the type, to ensure the values read from JSON are converted to lowercase.

volmedo · 2019-10-18T10:46:05Z

make sure all unserialized values are converted to lower case

Where is the unmarshalling being done? I suppose it is done by some library from k8s or the operator framework. If that is the case, is it enough to make the new type implement the Unmarshaler interface?

possibly a test ensuring that current CRs with a mixed-case strategy are correctly parsed

Where should this test live? Are there any other similar tests?

jpkrohling · 2019-10-18T12:11:15Z

Where is the unmarshalling being done?

It's done by the Operator SDK. It's sufficient to have a couple of methods, like these from the Options object:

jaeger-operator/pkg/apis/jaegertracing/v1/options.go

Lines 41 to 62 in d11973f

    
           func (o *Options) UnmarshalJSON(b []byte) error { 
        
           	var entries map[string]interface{} 
        
           	d := json.NewDecoder(bytes.NewReader(b)) 
        
           	d.UseNumber() 
        
           	if err := d.Decode(&entries); err != nil { 
        
           		return err 
        
           	} 
        
           	o.parse(entries) 
        
           	o.json = b 
        
           	return nil 
        
           } 
        
           // MarshalJSON specifies how to convert this object into JSON 
        
           func (o Options) MarshalJSON() ([]byte, error) { 
        
           	if len(o.json) == 0 && len(o.opts) == 0 { 
        
           		return []byte("{}"), nil 
        
           	} else if len(o.json) == 0 && len(o.opts) > 0 { 
        
           		return json.Marshal(o.opts) 
        
           	} 
        
           	return o.json, nil 
        
           }

I think you only need to implement the UnmarshalJSON, but needs confirmation.

We have some tests for the same object that could be used as reference:

jaeger-operator/pkg/apis/jaegertracing/v1/options_test.go

Lines 11 to 16 in d11973f

    
           func TestSimpleOption(t *testing.T) { 
        
           	o := Options{} 
        
           	o.UnmarshalJSON([]byte(`{"key": "value"}`)) 
        
           	args := o.ToArgs() 
        
           	assert.Equal(t, "--key=value", args[0]) 
        
           }

…ned ones Signed-off-by: volmedo <[email protected]>

jpkrohling

LGTM. There's only one minor comment about the import renaming for corev1 vs. jv1. If you don't want to change it in this PR, please create an issue so that we don't lose track of it.

jpkrohling · 2019-10-21T08:42:37Z

pkg/strategy/strategy.go

@@ -9,12 +9,13 @@ import (
 	"k8s.io/api/extensions/v1beta1"
 	rbac "k8s.io/api/rbac/v1"

+	jv1 "github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"


We typically use v1 for Jaeger, and corev1 for k8s.io/api/core/v1

Makes a lot of sense!

I saw that v1 was already in use and invented a different alias for Jaeger. Didn't think about the possibility of renaming the existing one :). Will do it now.

jpkrohling

Sorry, let me take back my approval :-) Just realized that it's still missing the serialization tests, to ensure backwards compatibility (which is probably why this is still WIP).

Signed-off-by: volmedo <[email protected]>

volmedo · 2019-10-21T09:12:08Z

Sorry, let me take back my approval :-) Just realized that it's still missing the serialization tests, to ensure backwards compatibility (which is probably why this is still WIP).

Yes, it's WIP because of that. I added three items to a todo list for the PR and I began with the last one instead of the first one :).

Signed-off-by: volmedo <[email protected]>

volmedo · 2019-10-21T21:40:25Z

I added the unmarshalling code and the corresponding tests.

I implemented encoding.UnmarshalText for the type instead of json.UnmarshalJSON since we are dealing with a single string value. The docs say that json.Unmarshal uses encoding.UnmarshalText if json.UnmarshalJSON is not available.

Aside from the tests, I also did some manual tests applying several modified yaml files and everything worked as expected. BTW, is there any end to end test that involves applying malformed deployment yaml files?

I didn't include code for marshalling. Since the underlying type is string, marshalling comes for free. I wrote tests to check this and to add protection for custom marshalling code that could be added in the future.

I've also removed the WIP label from the PR as it should be ready now :)

jpkrohling

LGTM. Once the nil pointer check is clarified, this can be merged!

jpkrohling · 2019-10-22T08:54:26Z

pkg/apis/jaegertracing/v1/deployment_strategy.go

+// UnmarshalText implements encoding.TextUnmarshaler to ensure that JSON values in the
+// strategy field of JSON jaeger specs are interpreted in a case-insensitive manner
+func (ds *DeploymentStrategy) UnmarshalText(text []byte) error {
+	if ds == nil {


This is interesting. I'm probably missing something obvious, but how would this be triggered? Wouldn't a nil object fail with a nil reference before reaching this code? Can you come up with a test case for this?

It's not obvious. It is possible for the function to be called on a nil receiver when interfaces are involved (as is the case here).

I was digging into the docs and I had a look at how json.RawMessage.UnmarshalJSON is implemented. The check for nil felt strange, so I did a bit of research and came up with this https://tour.golang.org/methods/12

Wonderful Go! :)

BTW, I guess this kind of check should be implemented in other types implementing interfaces for additional safety. Options come to my mind, implementing both json.Marshaler and json.Unmarshaler interfaces, but I'm sure there are several others.

TIL! I'm merging this one. Would you mind creating an issue to do the same nil check in the other relevant types?

jpkrohling · 2019-10-22T14:00:08Z

Thanks for your contribution!

volmedo force-pushed the spec-strategy-type branch 2 times, most recently from ea92767 to 3949e62 Compare October 17, 2019 12:18

volmedo added 4 commits October 17, 2019 14:20

Add DeploymentStrategy type and apply to JaegerSpec.Strategy

4dcf62a

Signed-off-by: volmedo <[email protected]>

Add constants for DeploymentStrategy

5e43380

Signed-off-by: volmedo <[email protected]>

Adapt code to the new type

5843126

Signed-off-by: volmedo <[email protected]>

Complete comments

cafa6ce

Signed-off-by: volmedo <[email protected]>

volmedo force-pushed the spec-strategy-type branch from 3949e62 to cafa6ce Compare October 17, 2019 12:21

jpkrohling reviewed Oct 17, 2019

View reviewed changes

volmedo changed the title ~~New DeploymentStrategy type for JaegerSpec.Strategy~~ [WIP] New DeploymentStrategy type for JaegerSpec.Strategy Oct 18, 2019

Remove strategy type constants in pkg/strategy and use the newly defi…

0a29840

…ned ones Signed-off-by: volmedo <[email protected]>

jpkrohling approved these changes Oct 21, 2019

View reviewed changes

jpkrohling reviewed Oct 21, 2019

View reviewed changes

Change package aliases to match convention

a8850d6

Signed-off-by: volmedo <[email protected]>

Add unmarshaling code and tests

ae31e2b

Signed-off-by: volmedo <[email protected]>

volmedo force-pushed the spec-strategy-type branch from 0da9d6d to ae31e2b Compare October 21, 2019 21:37

volmedo changed the title ~~[WIP] New DeploymentStrategy type for JaegerSpec.Strategy~~ New DeploymentStrategy type for JaegerSpec.Strategy Oct 21, 2019

volmedo requested a review from jpkrohling October 21, 2019 21:42

jpkrohling approved these changes Oct 22, 2019

View reviewed changes

jpkrohling merged commit 281ce62 into jaegertracing:master Oct 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New DeploymentStrategy type for JaegerSpec.Strategy #704

New DeploymentStrategy type for JaegerSpec.Strategy #704

volmedo commented Oct 17, 2019 •

edited

Loading

jpkrohling left a comment

jpkrohling Oct 17, 2019

volmedo Oct 18, 2019

jpkrohling Oct 17, 2019

volmedo commented Oct 18, 2019

jpkrohling commented Oct 18, 2019

jpkrohling left a comment

jpkrohling Oct 21, 2019

volmedo Oct 21, 2019

jpkrohling left a comment

volmedo commented Oct 21, 2019

volmedo commented Oct 21, 2019

jpkrohling left a comment

jpkrohling Oct 22, 2019

volmedo Oct 22, 2019

volmedo Oct 22, 2019

jpkrohling Oct 22, 2019

volmedo Oct 22, 2019

jpkrohling commented Oct 22, 2019

New DeploymentStrategy type for JaegerSpec.Strategy #704

New DeploymentStrategy type for JaegerSpec.Strategy #704

Conversation

volmedo commented Oct 17, 2019 • edited Loading

To do

jpkrohling left a 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

volmedo commented Oct 18, 2019

jpkrohling commented Oct 18, 2019

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

volmedo commented Oct 21, 2019

volmedo commented Oct 21, 2019

jpkrohling left a 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

jpkrohling commented Oct 22, 2019

volmedo commented Oct 17, 2019 •

edited

Loading