Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid touching the original structure of the options #523

Merged
merged 1 commit into from
Jul 16, 2019
Merged

Avoid touching the original structure of the options #523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions pkg/apis/jaegertracing/v1/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
// Options defines a common options parameter to the different structs
type Options struct {
opts map[string]string
json []byte
}

// NewOptions build a new Options object based on the given map
Expand Down Expand Up @@ -46,13 +47,18 @@ func (o *Options) UnmarshalJSON(b []byte) error {
}

o.parse(entries)
o.json = b
return nil
}

// MarshalJSON specifies how to convert this object into JSON
func (o Options) MarshalJSON() ([]byte, error) {
b, err := json.Marshal(o.opts)
return b, err
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
}

func (o *Options) parse(entries map[string]interface{}) {
Expand Down
17 changes: 17 additions & 0 deletions pkg/apis/jaegertracing/v1/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,20 @@ func TestExposedMap(t *testing.T) {
o.UnmarshalJSON([]byte(`{"cassandra": {"servers": "cassandra:9042"}}`))
assert.Equal(t, "cassandra:9042", o.Map()["cassandra.servers"])
}

func TestMarshallRaw(t *testing.T) {
json := []byte(`{"cassandra": {"servers": "cassandra:9042"}}`)
o := NewOptions(nil)
o.json = json
bytes, err := o.MarshalJSON()
assert.NoError(t, err)
assert.Equal(t, bytes, json)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the behavior before this change? {"cassandra.servers": "cassandra:9042"}?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change this PR does is on marshal and unmarshall, previously it flats all options into condensed form, now I don't do that, because it could produce Jaeger resources with mixed condensed and structured form of options, I just leave the options intact.

So for this case will return the JSON in the way it is, this is: {"cassandra": {"servers": "cassandra:9042"}} , but from code perspective you can still access to a map with condensed form of properties.

I did in this way trying to preserve backwards compatibility, if someone is already using condensed form, it will work, same for users using the structured form of options.

Other option is to change this to map[string]string, but that breaks the compatibility for users that uses structured form

}

func TestMarshallEmpty(t *testing.T) {
o := NewOptions(nil)
json := []byte(`{}`)
bytes, err := o.MarshalJSON()
assert.NoError(t, err)
assert.Equal(t, bytes, json)
}
5 changes: 5 additions & 0 deletions pkg/apis/jaegertracing/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.