-
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
Fix default value behavior for channel plugins #205
Comments
In a nutshell, this allows omitting key value pairs from the configuration JSON given to Plugin.SetConfig, falling back to default values specified in struct tags. To avoid storing each default value twice, it's location was moved up to a "default" struct tag. By utilizing the already included defaults library and adding a small wrapper for plugin.Info, those defaults are now being set to the plugin.ConfigAttributes at runtime. Tests for this new wrapper function, plugin.NewInfo, and for one channel plugin were written. Furthermore, lots of lines have changed their indentation level. Thus, the diff looks greater and scarier as it is. Closes #205.
In a nutshell, this allows omitting key value pairs from the configuration JSON given to Plugin.SetConfig, falling back to default values specified in struct tags. To avoid storing each default value twice, it's location was moved up to a "default" struct tag. By utilizing the already included defaults library and adding a small wrapper for plugin.Info, those defaults are now being set to the plugin.ConfigAttributes at runtime. Tests for this new wrapper function, plugin.NewInfo, and for one channel plugin were written. Furthermore, lots of lines have changed their indentation level. Thus, the diff looks greater and scarier as it is. Closes #205.
In a nutshell, this allows omitting key value pairs from the configuration JSON given to Plugin.SetConfig, falling back to default values specified in struct tags. To avoid storing each default value twice, it's location was moved up to a "default" struct tag. By utilizing the already included defaults library and adding a small wrapper for plugin.Info, those defaults are now being set to the plugin.ConfigAttributes at runtime. Tests for this new wrapper function, plugin.NewInfo, and for one channel plugin were written. Furthermore, lots of lines have changed their indentation level. Thus, the diff looks greater and scarier as it is. Closes #205.
In a nutshell, this allows omitting key value pairs from the configuration JSON given to Plugin.SetConfig, falling back to default values specified in struct tags. To avoid storing each default value twice, it's location was moved up to a "default" struct tag. By utilizing the already included defaults library and adding a small wrapper for plugin.Info, those defaults are now being set to the plugin.ConfigAttributes at runtime. Tests for this new wrapper function, plugin.NewInfo, and for one channel plugin were written. Furthermore, lots of lines have changed their indentation level. Thus, the diff looks greater and scarier as it is. Closes #205.
Can you please elaborate on what this is supposed to achieve? Note that giving
Live example: https://go.dev/play/p/Nl7S7kAXZjX |
Initially, this was intended to indicate that the user does not want to use the default value (if any) of an optional field of a specific channel. However, this does not seem to work if GO does not decode JSON |
Valid point. However, when first setting a default (or fallback) value, the struct is already partially populated and gets only overwritten when a user-configured value is stored in the JSON. As no further reconfiguration takes place, this shouldn't be an issue. |
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 daemon team and the web team, summarized in #205, web does not store or stores default values as JSON "null" values. The Go JSON unmarshaller does not overwrite existing field values for JSON nulls. 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. Closes #205.
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 daemon team and the web team, summarized in #205, web does not store or stores default values as JSON "null" values. The Go JSON unmarshaller does not overwrite existing field values for JSON nulls. 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. Closes #205.
In the meantime, #206 was generated to implement this in Icinga Notifications. However, as @yhabteab pointed out #206 (comment), the second requirement - a Unfortunately, this behavior does not work well together with Go's
It would be possible to work around this, but this makes third party channel plugin development way harder. For a concrete example, setting, i.e., the email channel's Furthermore, what should the channel plugin do for the Can we reconsider this and drop the |
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 daemon team and the web team, summarized in #205, web does not store or stores default values as JSON "null" values. The Go JSON unmarshaller does not overwrite existing field values for JSON nulls. 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. Closes #205.
We discussed this again with @nilmerg and came to dropping the " |
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.
We (@nilmerg, @yhabteab, and I) have just discussed this offline and came to the following design conclusion:
null
value results in an empty value for optional key value pairs.Those changes must be done in the daemon or, more precisely, the channel plugins and I am going to make the necessary changes tomorrow.
Originally posted by @oxzi in Icinga/icinga-notifications-web#154 (comment)
The text was updated successfully, but these errors were encountered: