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.

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

Adds json struct tags to mirror yaml ones #297

Merged
merged 4 commits into from May 10, 2021

Conversation

owen-d
Copy link
Contributor

@owen-d owen-d commented May 5, 2021

This PR includes json struct tags to mirror the yaml ones in config objects. This allows downstream projects to consume these with a consistent key structure regardless of format. I'm hoping this isn't too controversial :).

For context: I'd like to consolidate these so Grafana can more easily present a JSON API over the Alertmanager structs and seamlessly transition json<>yaml. The lack of tag synchronization combined with the <secret> redaction in marshaling code makes this a bit more difficult than I'd like.

Interestingly, this has already been done with Alertmanager but not with Prometheus.

Please let me know if this is reasonable, and if so, if you'd like me to add some marshaling testware.
/cc @roidelapluie

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@roidelapluie
Copy link
Member

Please add the marshal json to hide the secrets.

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d
Copy link
Contributor Author

owen-d commented May 5, 2021

Before I get fancy and try to align the underlying json/yaml {un,}marshaling code, I pushed up some support for synchronizing secret, url, and http config validations.

Let me know if I should continue or if this is sufficient, thanks.

@@ -35,6 +40,11 @@ func (s *Secret) UnmarshalYAML(unmarshal func(interface{}) error) error {
return unmarshal((*plain)(s))
}

// MarshalJSON implements the json.Marshaler interface for Secret.
func (s Secret) MarshalJSON() ([]byte, error) {
return json.Marshal(secretToken)
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the same as in marshal yaml, return nil when empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally copied this from Alertmanager (although these types should probably be defined solely in common and imported there) and wanted to keep parity with the implementation there.

As for returning nil, we cannot actually do this as it's unsupported for the MarshalJSON method, see example

I can, however, replace it with something like the following if you think it's better to show "" for an empty secret when the field is not specified as omitempty.

// MarshalJSON implements the json.Marshaler interface for Secret.
func (s Secret) MarshalJSON() ([]byte, error) {
	if len(s) == 0 {
		return json.Marshal("")
	}
	return json.Marshal(secretToken)
}

How would you like me to proceed?

Copy link
Member

Choose a reason for hiding this comment

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

marshalling to "" seems OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@@ -36,6 +37,7 @@ import (
"testing"
"time"

"github.com/stretchr/testify/require"
Copy link
Member

Choose a reason for hiding this comment

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

let's not depend on testify here, even for tests. Thanks.

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@roidelapluie roidelapluie merged commit c7c397c into prometheus:main May 10, 2021
@roidelapluie
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants