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

config: add json tags to config structs #1309

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Jakob3xD
Copy link

@Jakob3xD Jakob3xD commented Oct 16, 2024

This adds json tags to config struct fields. This is needed for us to reuse the structs in an internal kubernetes operator.

If just adding the tags is unwanted, I can also look into adding general support for json configs. We would need to add UnmarshalJSON functions like the UnmarshalYAML functions.

Used SED to add the json tags.

sed -i 's/`\(yaml:"\(.*\)"\)`/`\1 json:"\2"`/g' config/config.go

Closes #1308

config/config.go Outdated Show resolved Hide resolved
@electron0zero
Copy link
Member

If just adding the tags is unwanted, I can also look into adding general support for json configs. We would need to add UnmarshalJSON functions like the UnmarshalYAML functions.

thank you for sending this in, just tags are okay.

I am not sure if we want to add support for json configs, I don't see a big reason to add support for json configs.

@Jakob3xD
Copy link
Author

thank you for sending this in, just tags are okay.

Perfect, thank you.

Copy link
Member

@electron0zero electron0zero left a comment

Choose a reason for hiding this comment

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

lgtm, will wait for a week or so to give time to other maintainers to take a look before we merge it

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Seems fine to me.

@Jakob3xD Jakob3xD changed the title config: add json tags to config structs Draft: config: add json tags to config structs Oct 17, 2024
@Jakob3xD
Copy link
Author

Just adding the json tags won't work as it also needs custom Unmarshal functions. As the json support is not wanted? I might close the PR and do the changes in my fork :/
(Currently still working on the custom Unmarshal functions. Once it is done I would open an PR and you can decide if you want it or not?)

@electron0zero
Copy link
Member

also needs custom Unmarshal functions

we don't mind having the JSON Unmarshal functions, it would be useful to folks using Blackbox exporter as a library and build on top of Blackbox exporter like you are doing

What I meant was the full fledged support for configuring blackbox exporter via a json config file instead of YAML, in that side, will stick with YAML only.

@Jakob3xD Jakob3xD marked this pull request as draft October 18, 2024 10:59
@Jakob3xD
Copy link
Author

First step is done with the custom Unmarshal functions. I might need to add some Marshal functions as well.
Can already tell that the current json lib is a pain as it does not support inline fields.

If wished I can also generate the json test cases during the test by converting the original yml test case to a json file.
Would remove the "duplicate" test data.

@electron0zero
Copy link
Member

If wished I can also generate the json test cases during the test by converting the original yml test case to a json file.
Would remove the "duplicate" test data.

yes, that would be better then the duplicate config files.

@Jakob3xD Jakob3xD changed the title Draft: config: add json tags to config structs config: add json tags to config structs Oct 21, 2024
@Jakob3xD Jakob3xD marked this pull request as ready for review October 21, 2024 09:10
@Jakob3xD
Copy link
Author

As an additional question.
Is it intended that the Config should not be Marshaled? Asking because when marshaling the Config all the Default values of the other modules are printed as well and are not omitted. From what I know right now, there is no nice way to Marshal the config. To solve this we could make all the Modules pointers that would get omitted.

@electron0zero
Copy link
Member

Is it intended that the Config should not be Marshaled? Asking because when marshaling the Config all the Default values of the other modules are printed as well and are not omitted.

I don't think we thought about this before, this might be the default. changing the way config is Marshaled would need a little more discussion.

if you want to change that marshaled, I would recommend creating an issue to discuss this with everyone, and if we agree on changing, i would prefer it to be it's own PR.

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.

Feature: Add json Tags
3 participants