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 autoEnabled plugin JSON field to auto enable App plugins and add configuration link to menu by default #31354

Merged
merged 7 commits into from
Feb 25, 2021

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Feb 19, 2021

  • App plugins no longer get a "Plugin Config" menu option added automatically
  • If you navigate to the plugin via the Plugins list it will default to the config page (if it's an app and export a config ctrl)
  • An app plugin that wants the Plugin Config to remain in the main menu can add it as an entry in the plugin.json (includes) like any plugin page
  • New option plugin.json autoEnable option that makes the plugin be enabled by default (making plugins be auto-enabled requires a bit more changes so we can disable any plugin). Can wait for v8

@torkelo torkelo requested a review from a team as a code owner February 19, 2021 13:54
@torkelo torkelo requested review from wbrowne and ying-jeanne and removed request for a team February 19, 2021 13:54
@@ -15,7 +15,8 @@ import (

type AppPlugin struct {
FrontendPluginBase
Routes []*AppPluginRoute `json:"routes"`
Routes []*AppPluginRoute `json:"routes"`
NoConfig bool `json: "noConfig"`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe noConfigPage or noConfigPageInNav instead? noConfig feels very generic.

Copy link
Member Author

Choose a reason for hiding this comment

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

yea had the same thought

Copy link
Member

@dprokop dprokop 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.

We should add an entry for the new field in docs/sources/developers/plugins/plugin.schema.json.

Also just to confirm, we should still be able to visit the config page from the plugins list view? Does this change only remove it from the sidebar nav?

@torkelo
Copy link
Member Author

torkelo commented Feb 19, 2021

Oh i think this might need another change to auto enable the plugin , or maybe that exists already need to check

@torkelo torkelo requested review from a team, peterholmberg and natellium and removed request for a team February 22, 2021 15:28
@natellium
Copy link
Contributor

@torkelo should we add this to the plugins platform project and set the milestone to 7.5?

@torkelo
Copy link
Member Author

torkelo commented Feb 22, 2021

@natellium yes

@torkelo torkelo added this to the 7.5.0 milestone Feb 22, 2021
@@ -64,7 +64,10 @@ func GetEnabledPlugins(orgId int64) (*EnabledPlugins, error) {
}

for pluginId, app := range Apps {
if b, ok := pluginSettingMap[pluginId]; ok {
if app.AutoEnabled {
Copy link
Member

Choose a reason for hiding this comment

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

This won't actually enable apps though correct? Or at least, it won't set the plugin data - It will just pin it in the nav, so maybe AutoPinned/AutoPinnedToNav is a better name?

Copy link
Member Author

Choose a reason for hiding this comment

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

it will once I figured out how, at least that is the plan, but was harder than I expected

@@ -122,6 +122,14 @@
"hiddenQueries": {
"type": "boolean"
},
"noConfigPage": {
"type": "boolean",
"description": "For app plugins that have no config page"
Copy link
Member

Choose a reason for hiding this comment

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

A little bit confused by this - Does this mean that the app can't be configured at all?

Copy link
Member Author

@torkelo torkelo Feb 22, 2021

Choose a reason for hiding this comment

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

just means they don't have a default config page

@ryantxu
Copy link
Member

ryantxu commented Feb 22, 2021

Ugg -- for v8, can the sidebar menu be defined programatically? Why do we need it in plugin.json?

Ideally we can remove all the sidemenu options from plugin.json

@torkelo
Copy link
Member Author

torkelo commented Feb 22, 2021

@ryantxu I do not think the top level navigation items should require loading async external plugins during frontend rendering, that I think should be declaratively defined in the plugin.json along with user role permission restrictions .

Otherwise the sidemenu will require some loading state and things plopping in after some plugin loading phase? feels bad

@torkelo
Copy link
Member Author

torkelo commented Feb 24, 2021

@ryantxu I changed this a bit, so it's a bit more of a breaking change but I think it's ok.

  • App plugins no longer get a "Plugin Config" menu option added automatically
  • If you navigate to the plugin via the Plugins list it will default to the config page (if it's an app and export a config ctrl)
  • An app plugin that wants the Plugin Config to remain in the main menu can add it as an entry in the plugin.json (includes) like any plugin page
  • New option plugin.json autoEnable option that makes the plugin be enabled by default (making plugins be auto-enabled requires a bit more changes so we can disable any plugin). Can wait for v8

what do you think?

Copy link
Member

@ryantxu ryantxu left a comment

Choose a reason for hiding this comment

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

This is much better! although we should still be open to more breaking changes in 8.

@torkelo torkelo added the area/grafana/ui Issues that belong to components in the @grafana/ui library label Feb 25, 2021
@torkelo torkelo changed the title AppPlugins: Options to disable showing config page in nav AppPlugins: Change to plugin menu, do not add Plugin Config to menu by default, and a new option to auto enable app plugins Feb 25, 2021
@torkelo torkelo merged commit 584886f into master Feb 25, 2021
@torkelo torkelo deleted the app-plugins-config branch February 25, 2021 09:00
@wbrowne wbrowne changed the title AppPlugins: Change to plugin menu, do not add Plugin Config to menu by default, and a new option to auto enable app plugins Plugins: Add autoEnabled plugin JSON field to auto enable App plugins and add configuration link to menu by default Mar 3, 2021
@mikhail-vl
Copy link

mikhail-vl commented Mar 11, 2021

@torkelo @ryantxu I understand that autoEnabled feature will be fully implemented for v8. In 7.5-beta1 panels are auto-imported and application enabled, but dashboards are missing. Is it going to be fixed in v8, should I open an issue?

@torkelo
Copy link
Member Author

torkelo commented Mar 11, 2021

Open an issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/grafana/ui Issues that belong to components in the @grafana/ui library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants