New DeploymentStrategy type for JaegerSpec.Strategy

[volmedo]

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

[jpkrohling]

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

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

Apart from that, LGTM.

[jpkrohling]

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

[volmedo]

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

[jpkrohling]

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

[volmedo]

1. 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?

2. 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]

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])
	}

[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]

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

[volmedo]

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).

[volmedo]

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 :).

[volmedo]

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]

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?

[volmedo]

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! :)

[volmedo]

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.

[jpkrohling]

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

[volmedo]

Will do!

[jpkrohling]

Thanks for your contribution!