Configs API: Allow POST configs in YAML format. #2181
Conversation
fixes cortexproject#2170 Signed-off-by: Wing924 <weihe924stephen@gmail.com>
Signed-off-by: Wing924 <weihe924stephen@gmail.com>
|
Removed myself and Goutham as reviewers cause this is @jtlisi area of expertise. I can jump in as second reviewer, if necessary. |
|
@jtlisi Could you review this? |
pkg/configs/api/api.go
Outdated
| @@ -111,7 +113,7 @@ func (a *API) setConfig(w http.ResponseWriter, r *http.Request) { | |||
| logger := util.WithContext(r.Context(), util.Logger) | |||
|
|
|||
| var cfg configs.Config | |||
| if err := json.NewDecoder(r.Body).Decode(&cfg); err != nil { | |||
| if err := yaml.NewDecoder(r.Body).Decode(&cfg); err != nil { | |||
There was a problem hiding this comment.
Instead of assuming JSON is a subset of YAML for this to work (works most of the time, but there are corner cases), can you use a content-type head to detect YAML? And have JSON be the default.
Signed-off-by: Wing924 <weihe924stephen@gmail.com>
pkg/configs/api/api.go
Outdated
| FormatYAML = "yaml" | ||
| ) | ||
|
|
||
| func ParseConfigFormat(v string, defaultFormat string) string { |
There was a problem hiding this comment.
No need to expose this function. Can be parseConfigFormat()
There was a problem hiding this comment.
If I don't expose this function, I can't write test against this function.
There was a problem hiding this comment.
This function is tested within the same package pkg/configs/api/api_test.go so you shouldn't have any problem testing it even if unexported (because unexported functions are still visibile within the same package).
pkg/configs/api/api.go
Outdated
| FormatYAML = "yaml" | ||
| ) | ||
|
|
||
| func ParseConfigFormat(v string, defaultFormat string) string { |
There was a problem hiding this comment.
This function is tested within the same package pkg/configs/api/api_test.go so you shouldn't have any problem testing it even if unexported (because unexported functions are still visibile within the same package).
pkg/configs/api/api.go
Outdated
| // should not be here | ||
| level.Error(logger).Log("msg", "unknown Format") | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) |
There was a problem hiding this comment.
| // should not be here | |
| level.Error(logger).Log("msg", "unknown Format") | |
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| // should never reach this point | |
| level.Error(logger).Log("msg", "unexpected error detecting the config format") | |
| http.Error(w, err.Error(), http.StatusInternalServerError) |
pkg/configs/api/api.go
Outdated
| // should not be here | ||
| level.Error(logger).Log("msg", "unknown Format") | ||
| http.Error(w, err.Error(), http.StatusBadRequest) |
There was a problem hiding this comment.
| // should not be here | |
| level.Error(logger).Log("msg", "unknown Format") | |
| http.Error(w, err.Error(), http.StatusBadRequest) | |
| // should never reach this point | |
| level.Error(logger).Log("msg", "unexpected error detecting the config format") | |
| http.Error(w, err.Error(), http.StatusInternalServerError) |
|
@pracucci since |
Hold on. Why "api_test.go use api_test package"? I think that's a mistake. It should just use the same package of api.go |
|
@pracucci You're right. There is no reason to use |
What this PR does:
allow post configs API in YAML format
allow get configs API in YAML format
Which issue(s) this PR fixes:
Fixes #2170
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]