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

Jump to bottom

Omit empty config fields and show regex upon re-marshalling to elide secrets #864

Merged
merged 2 commits into from
Jun 20, 2017
Merged

Omit empty config fields and show regex upon re-marshalling to elide secrets #864

merged 2 commits into from
Jun 20, 2017

Conversation

conr
Copy link
Contributor

@conr conr commented Jun 15, 2017

Fixes #862

mxinden
mxinden reviewed Jun 15, 2017
View reviewed changes
config/notifiers.go Outdated

// Catches all undefined fields and must be empty after parsing.
XXX map[string]interface{} `yaml:",inline" json:"-"`
XXX map[string]interface{} `yaml:",inline" ,omitemptyjson:"-"`
Copy link
Member

Choose a reason for hiding this comment

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

Is this a typo? I can't find omitemptyjson in the specs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that's a typo!

@conr conr force-pushed the marshal-config branch from a92f4bd to 62dd545 Compare June 15, 2017 11:15
@stuartnelson3
Copy link
Contributor

@grobie can you give this a try?

@grobie
Copy link
Member

grobie commented Jun 15, 2017

Can we write a test instead?

@conr conr force-pushed the marshal-config branch from c636530 to f52a366 Compare June 15, 2017 15:45
@conr
Copy link
Contributor Author

conr commented Jun 15, 2017

Not sure about default values yet but this should fix and test for the issue regarding regexps printing as {} instead of the actual regular expression and unset fields shown as "" or null.

@conr
Copy link
Contributor Author

conr commented Jun 20, 2017

@stuartnelson3 @grobie

Any thoughts?

stuartnelson3
stuartnelson3 approved these changes Jun 20, 2017
View reviewed changes
Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

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

LGTM, but there is what appears to be some sort of break on the status page that is unrelated.

@stuartnelson3 stuartnelson3 merged commit b5ad65f into prometheus:master Jun 20, 2017
@stuartnelson3
Copy link
Contributor

Disregard the comment about breakage, it was because your version of alertmanager was missing updates from mainline

@mxinden mxinden mentioned this pull request Jul 15, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden left review comments

@stuartnelson3 stuartnelson3 stuartnelson3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@conr @stuartnelson3 @grobie @mxinden