-
Notifications
You must be signed in to change notification settings - Fork 1
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
Channel default value by changing default logic #206
Conversation
d7873da
to
fcf8531
Compare
fcf8531
to
7ff9196
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LFTM!
7ff9196
to
40023a4
Compare
Rebased after switch to icinga-go-library. |
Have you considered doing it the other way around, i.e. applying the defaults from I think that this can be done in a much simpler way, without any custom use of reflection even: Iterate over |
0dbbfd9
to
0ce82f4
Compare
I liked @julianbrost's suggestion and worked on it. The new design reduces the change's size and is, imo, more readable. Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code change looks fine (and quite a lot nicer than the previous version IMHO), actually using default values by default sounds sane, did not break anything in my test setup including after saving the config from web again, so looks fine to me. Nonetheless, @yhabteab please also have another look at this as you were involved in the discussions mentioned in #205.
I'm a bit confused then, given that unmarshaling a JSON null value into a GO scalar type has no effect, how does the user clear optional default values then? Web does store null values to signify that a user does not want to use a default value of that field (if any).
Or to be more specific, how do you deal with the second point of the referenced issue? |
For the record, as this changes the channel logic a bit and introduces another helper function, it should either be included in #236 if this got merged first or be an update in this PR otherwise. |
0ce82f4
to
8b8bd63
Compare
In a nutshell, the newly introduced plugin.PopulateDefaults function populates all fields of a Plugin-implementing struct with those fields from Info.ConfigAttributes where ConfigOption.Default is set. Thus, a single function call before parsing the user-submitted configuration within the Plugin.SetConfig method sets default values. As a result of the discussion between the Go and the Web team, summarized in #205, Web does not store key-value pairs to be omitted. Prior, an already JSON-encoded version of the ConfigOption slice was present in plugin.Info. Thus, this data wasn't easily available anymore. As the new code now needs to access this field, it was changed and a custom sql driver.Valuer was implemented for a slice type. While working on this, all ConfigOptions were put in the same order as the struct fields. Requires <Icinga/icinga-notifications-web#230>. Closes #205.
8b8bd63
to
53448cb
Compare
Requires Icinga/icinga-notifications-web#230.
Closes #205.