[UPDATED] Added json tags to KeyValueConfig struct to behave consistently with the StreamConfig one #1630
Conversation
Signed-off-by: Mattia Barbisan <pricelessrabbit@gmail.com>
There was a problem hiding this comment.
Thanks for the PR! I have one suggestion regarding the tag name.
Other than that, I don't think omitempty matters a lot in that case since we never really serialize the config from within the library. If anything, I would probably use omitempty to represent optionality of those fields, similarly to how we have it in object store - i.e. only bucket name would not have it.
jetstream/kv.go
Outdated
|
|
||
| // Replicas is the number of replicas to keep for the KeyValue store in | ||
| // clustered jetstream. Defaults to 1, maximum is 5. | ||
| Replicas int | ||
| Replicas int `json:"replicas"` |
There was a problem hiding this comment.
To be consistent with stream config, this should probably be num_replicas
There was a problem hiding this comment.
i just pushed the fix with num_replicas and also changed omitempty like the object storage ones 🐰
- udpated "omitempty" options to match the object store config ones Signed-off-by: Mattia Barbisan <pricelessrabbit@gmail.com>
kv.go
Outdated
| Description string `json:"description,omitempty"` | ||
| MaxValueSize int32 `json:"max_value_size,omitempty"` | ||
| History uint8 `json:"history,omitempty"` | ||
| TTL time.Duration `json:"ttl,omitempty,omitempty"` |
There was a problem hiding this comment.
Sorry about that, I did not notice at first, you have doubled omitempty here.
There was a problem hiding this comment.
sry, fixed!
Signed-off-by: Mattia Barbisan <pricelessrabbit@gmail.com>
This PR add json tags to the
KeyValueConfigstruct like the ones already present inStreamConfig.This is useful when working with KV buckets because currently you need a map or support struct to marshal / unmarshal a bucket config to / from JSON.
StreamConfigtags as reference.omitemptyoption for the fields that have it inStreamConfig, but a check is needed for the other ones.