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

Plugins: Add hide_angular_deprecation setting #79296

Merged
merged 6 commits into from
Dec 12, 2023

Conversation

xnyo
Copy link
Member

@xnyo xnyo commented Dec 11, 2023

What is this feature?

This PR adds an hide_angular_deprecation to [plugins] config section. This is a comma-separated list that acts the same as specifying the hide_angular_deprecation plugin setting.

This (new):

[plugins]
hide_angular_deprecation = SOME-ID,ANOTHER-ID

Is the same as this (old):

[plugin.SOME-ID]
hide_angular_deprecation = true

[plugin.ANOTHER_ID]
hide_angular_deprecation = true

This PR also removes the old way of setting the hide_angular_deprecation flag (via plugin settings). This was introduced in #77026 targeting 10.3.x so it's unreleased.

Why do we need this feature?

The main advantage of the new method is that it is possible to set the value via environment variables:

GF_PLUGINS_HIDE_ANGULAR_DEPRECATION = SOME-ID,ANOTHER-ID

Who is this feature for?

Grafana users.

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@xnyo xnyo self-assigned this Dec 11, 2023
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.3.x milestone Dec 11, 2023
@xnyo xnyo changed the title WIP: Plugins: Add hide_angular_deprecation plugin setting config Plugins: Add hide_angular_deprecation plugin setting config Dec 11, 2023
@xnyo xnyo added add to changelog no-backport Skip backport of PR labels Dec 11, 2023
@xnyo xnyo changed the title Plugins: Add hide_angular_deprecation plugin setting config Plugins: Add hide_angular_deprecation setting Dec 11, 2023
@xnyo xnyo marked this pull request as ready for review December 11, 2023 12:12
@xnyo xnyo requested review from torkelo and a team as code owners December 11, 2023 12:12
@xnyo xnyo requested review from wbrowne, marefr, andresmgot, papagian, undef1nd and suntala and removed request for a team December 11, 2023 12:12
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

The main advantage of the new method is that it is possible to set the value via environment variables

Hmm that should be possible with the other method as well 🤔 But there's a function needed to be call or something to populate from env.

@xnyo
Copy link
Member Author

xnyo commented Dec 11, 2023

The main advantage of the new method is that it is possible to set the value via environment variables

Hmm that should be possible with the other method as well 🤔 But there's a function needed to be call or something to populate from env.

The function that overrides config from env vars is the following:

func applyEnvVariableOverrides(file *ini.File) error {
appliedEnvOverrides = make([]string, 0)
for _, section := range file.Sections() {
for _, key := range section.Keys() {
envKey := EnvKey(section.Name(), key.Name())
envValue := os.Getenv(envKey)
if len(envValue) > 0 {
key.SetValue(envValue)
appliedEnvOverrides = append(appliedEnvOverrides, fmt.Sprintf("%s=%s", envKey, RedactedValue(envKey, envValue)))
}
}
}
return nil
}

Used here:

// apply environment overrides
err = applyEnvVariableOverrides(parsedFile)
if err != nil {
return nil, err
}

It uses the defaults.ini file to determine which env vars to read, so they won't get populated correctly if we have to consume them from core like here:

p.Angular.HideDeprecation = a.cfg.PluginSettings[p.ID]["hide_angular_deprecation"] == "true"


There's also DynamicSection:

grafana/pkg/setting/setting.go

Lines 1397 to 1417 in ea36336

type DynamicSection struct {
section *ini.Section
Logger log.Logger
}
// Key dynamically overrides keys with environment variables.
// As a side effect, the value of the setting key will be updated if an environment variable is present.
func (s *DynamicSection) Key(k string) *ini.Key {
envKey := EnvKey(s.section.Name(), k)
envValue := os.Getenv(envKey)
key := s.section.Key(k)
if len(envValue) == 0 {
return key
}
key.SetValue(envValue)
s.Logger.Info("Config overridden from Environment variable", "var", fmt.Sprintf("%s=%s", envKey, RedactedValue(envKey, envValue)))
return key
}

But it requires to state the section name, like this:

cfg.SectionWithEnvOverrides("section_name").Key("some_key").MustBool(false)

and we have one section per plugin ID, and it gets more complicated/confusing in my opinion as we also have the pkg/plugins-specific config

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

LGTM

@andresmgot
Copy link
Contributor

The main advantage of the new method is that it is possible to set the value via environment variables

Hmm that should be possible with the other method as well 🤔 But there's a function needed to be call or something to populate from env.

I guess it's worth mentioning that on a previous conversation, we agreed on only putting configuration relevant to the plugin code under the [plugin.id] section (e.g. configuration read on the plugin backend) and let configuration relevant to Grafana core (like hide_angular_deprecation) somewhere else (in this case the plugins section).

As @xnyo mentions, there is a bug / missing feature that prevents from mapping env vars to plugin specific config. I think we should create an issue to track that since it may be necessary for some other config. Do you mind opening that with your finds @xnyo ?

conf/defaults.ini Show resolved Hide resolved
@xnyo
Copy link
Member Author

xnyo commented Dec 11, 2023

Do you mind opening that with your finds @xnyo ?

Will do :)

@marefr
Copy link
Member

marefr commented Dec 12, 2023

As @xnyo mentions, there is a bug / missing feature that prevents from mapping env vars to plugin specific config. I think we should create an issue to track that since it may be necessary for some other config. Do you mind opening that with your finds @xnyo ?

It uses the defaults.ini file to determine which env vars to read, so they won't get populated correctly if we have to consume them from core like here:

I'm pretty sure we used to support that when I introduced support for plugin config, #23451. So yes, the reason is probably we nowadays do p.Angular.HideDeprecation = a.cfg.PluginSettings[p.ID]["hide_angular_deprecation"] == "true" . Creating an issue sounds good 👍

Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@xnyo xnyo enabled auto-merge (squash) December 12, 2023 09:10
@xnyo xnyo merged commit f76b9f2 into main Dec 12, 2023
13 checks passed
@xnyo xnyo deleted the giuseppe/hide-angular-deprecation-plugin-ids-config branch December 12, 2023 09:20
@summerwollin summerwollin modified the milestones: 10.3.x, 10.3.0 Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants