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

Feature: Allow to disable a plugin #74840

Merged
merged 5 commits into from Sep 14, 2023
Merged

Feature: Allow to disable a plugin #74840

merged 5 commits into from Sep 14, 2023

Conversation

andresmgot
Copy link
Contributor

What is this feature?

Add the possibility to skip loading a plugin, including core plugins (but not limited to those).

Why do we need this feature?

An operator may want to disable a plugin.

Who is this feature for?

Grafana operators.

Which issue(s) does this PR fix?:

Fixes https://github.com/grafana/grafana-operator-experience-squad/issues/614

Special notes for your reviewer:

Note that I have skipped adding a feature flag for this featue since it won't be used unless it's configured. Let me know if you still think a feature flag is necessary.

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.

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

I think this looks good

Copy link
Member

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Copy link
Member

@wbrowne wbrowne left a comment

Choose a reason for hiding this comment

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

I think we could probably just do this within pluginsintegration, no? We can introduce the step there in here https://github.com/grafana/grafana/blob/main/pkg/services/pluginsintegration/pipeline/steps.go and then we also don't have to introduce a new config to the plugins config struct.

WDYT?

Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

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

docs look good! Thanks for the contribution!

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 with a nit

@@ -1478,6 +1478,8 @@ public_key_retrieval_disabled = false
# Force download of the public key for verifying plugin signature on startup. If disabled, the public key will be retrieved every 10 days.
# Requires public_key_retrieval_disabled to be false to have any effect.
public_key_retrieval_on_startup = false
# Enter a comma-separated list of plugin identifiers to skip loading (including core plugins). These plugins will be hidden in the catalog.
skip_plugins =
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Would perhaps suggest renaming it to disabled_plugins. Something with "skip" that does feel weird to read for me without being able to pinpoint exactly why 😄

@andresmgot
Copy link
Contributor Author

I think we could probably just do this within pluginsintegration, no?

Right, I always forget 👍

Nit. Would perhaps suggest renaming it to disabled_plugins

Sounds good to me 👍

@andresmgot
Copy link
Contributor Author

and then we also don't have to introduce a new config to the plugins config struct.

@wbrowne as far as I can tell, the config change is still necessary because it's what's received in the pipeline but let me know if I am missing something.

@wbrowne
Copy link
Member

wbrowne commented Sep 14, 2023

and then we also don't have to introduce a new config to the plugins config struct.

@wbrowne as far as I can tell, the config change is still necessary because it's what's received in the pipeline but let me know if I am missing something.

Ah yeah I was thinking just also inject Grafana's *setting.Cfg there. I just worry that compared to some other config, someone might expect to set that and it automatically work (but won't because you need the right step). But also not a deal breaker!

@andresmgot andresmgot merged commit 96b55ea into main Sep 14, 2023
14 checks passed
@andresmgot andresmgot deleted the skipPlugins branch September 14, 2023 10:58
@andresmgot andresmgot changed the title Feature: Allow to skip plugin loading Feature: Allow to disable a plugin Sep 14, 2023
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants