Skip to content
This repository has been archived by the owner on Apr 26, 2021. It is now read-only.

Fix yaml annotations for embedded type #33

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Fix yaml annotations for embedded type #33

merged 1 commit into from
Mar 23, 2021

Conversation

papagian
Copy link
Collaborator

This is required because we have to store (in the DB) the alert manager configuration in YAML.

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Looks good, but I left a few questions

@@ -184,7 +184,7 @@ type GettableUserConfig struct {
AlertmanagerConfig GettableApiAlertingConfig `yaml:"alertmanager_config" json:"alertmanager_config"`
}
type GettableApiAlertingConfig struct {
config.Config
Config `yaml:",inline"`
Copy link
Member

Choose a reason for hiding this comment

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

This looks fine, but you can still leave the config object as an embedded struct if you like, exactly like how we do in:

type ExtendedRuleNode struct {
	// note: this works with yaml v3 but not v2 (the inline tag isn't accepted on pointers in v2)
	*ApiRuleNode `yaml:",inline"`
	//GrafanaManagedAlert yaml.Node `yaml:"grafana_alert,omitempty"`
	GrafanaManagedAlert *ExtendedUpsertAlertDefinitionCommand `yaml:"grafana_alert,omitempty" json:"grafana_alert,omitempty"`
}

@@ -241,8 +241,17 @@ func (c *GettableApiAlertingConfig) Type() (backend Backend) {
return
}

// Config is the top-level configuration for Alertmanager's config files.
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to copy this struct over? Can't we continue to reference it as config.Config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The difference is the yaml:"-" tag for the Receivers field because otherwise it conflicts with the Receivers field of the PostableApiAlertingConfig struct.

@owen-d owen-d merged commit f4c9118 into master Mar 23, 2021
@owen-d owen-d deleted the fix-am-yaml branch March 23, 2021 13:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants