Plugins: Move alias support to plugin json (but still hardcoded) #75129

ryantxu · 2023-09-19T22:12:40Z

This is an update for #69874 and now targeting 10.2

This moves the alias support to plugin.json, but still throws an error if someone besides the hardcoded plugins try to set it.

Ideally we can update the hosted grafana plugin submission logic so it won't accept anything with alias unless we hardcode it there 🤷

but then release this with 10.2 -- that will mean that in the future there is a migration path for plugins like:
https://grafana.com/grafana/plugins/dlopes7-appdynamics-datasource/
https://grafana.com/grafana/plugins/alexanderzobnin-zabbix-app/

This also makes the alias an array, and matches the behavior in Registry.ts
https://github.com/grafana/grafana/blob/main/packages/grafana-data/src/utils/Registry.ts#L8

ryantxu · 2023-09-19T22:14:55Z

@wbrowne -- I think this is safe to merge as is, and when (hopefully soon!) we have checks in gcom for alias the hardcoded errors can be removed here

andresmgot

Some initial review. If we add a new field to the plugin.json docs/sources/developers/plugins/plugin.schema.json and docs/sources/developers/plugins/metadata.md needs to be updated too.

That been said, I think that if we are adding support for that in plugins, we need to:

Probably support more that one alias (for the third time a plugin ID changes)
Documentation about how to add a new alias (e.g. adding a new ID to the hardcoded list)

andresmgot · 2023-09-20T08:08:55Z

pkg/plugins/plugins.go

-		plugin.Alias = "phlare"
-	case "debug": // panel plugin used for testing
-		plugin.Alias = "debugX"
+	case "grafana-pyroscope-datasource":


Nit: can we move this switch-case to a map? I see this list growing and a switch-case is slower/more difficult to maintain.

this whole section should be removed -- the only reason it exists is that we are waiting for parallel logic to exist in gcom. We want to be careful that anyone is not able to publish a plugin with an alias that we do not notice until it is too late :)

Can't we just say - we implement logic in gcom and we make sure we dont publish any plugin or plugin updates that contain alias that we dont want and the ones we want can do it. Then we can (here in the core) trust that the ones that are published and have an alias in plugin.json are good to have an alias? Otherwise it will become hard to keep those hardcoded lists in sync.

@tolzhabayev -- exactly. I have the check here so we can get everything working here and get it working in GCOM in parallel.

GCOM Ticket https://github.com/grafana/grafana-com/issues/7467 to double check we have the same approach in mind

andresmgot · 2023-09-20T08:09:15Z

pkg/plugins/plugins.go

+		if plugin.Alias == "" {
+			return plugin, fmt.Errorf("expected alias to be set")
+		}


this custom check seems to be not needed?

i have no trouble removing it -- it just verifies that the alias was set the same from when we hardcoded it.

yes, my concern is that people (us) start adding custom logic here to do more things (that's also why I like the idea of just having a map with the aliases)

once we trust gcom... this whole section can be removed

andresmgot · 2023-09-20T08:10:26Z

public/app/plugins/panel/annolist/plugin.json

  "id": "annolist",
+  "alias": "ryantxu-annolist-panel",


shouldn't this be:

"id": "grafana-annolist-panel", "alias": "annolist",

?

we could do that -- but that is a different project :) The goal here is not to rename the ids, just setting alias in plugin.json rather than hardcoded

andresmgot · 2023-09-20T08:11:44Z

public/app/plugins/panel/debug/plugin.json

  "id": "debug",
+  "alias": "debugX",


I see this plugin changing the id again to grafana-debug-panel in the future so we may need to support a string array for alias(es)

for this panel it should not matter -- this is an alpha panel just used for testing. The "debugX" is just a way to make sure that this same strategy works for our panels as it does for datasources :)

ryantxu · 2023-09-20T16:18:26Z

@andresmgot -- I get your point on docs... but IIUC, we want to be very careful about when this happens. It is not something we want to promote widely, but is a release.

If we want to make it an array (even if it only supports a single item for now) that also seems reasonable, but previous discussions suggested it was not necessary.

andresmgot · 2023-09-21T08:33:28Z

@andresmgot -- I get your point on docs... but IIUC, we want to be very careful about when this happens. It is not something we want to promote widely, but is a release.

I am not sure then what we win with this? It's still needed to hardcode it and we would be adding a property to the plugin.json that it's not usable in the general case (and we don't want to expose it).

aocenas · 2023-09-21T08:50:16Z

pkg/plugins/plugindef/plugindef.cue

@@ -18,6 +18,10 @@ schemas: [{
 		id: string & strings.MinRunes(1)
 		id: =~"^([0-9a-z]+\\-([0-9a-z]+\\-)?(\(strings.Join([ for t in _types {t}], "|"))))|(alertGroups|alertlist|annolist|barchart|bargauge|candlestick|canvas|dashlist|debug|datagrid|gauge|geomap|gettingstarted|graph|heatmap|histogram|icon|live|logs|news|nodeGraph|piechart|pluginlist|stat|state-timeline|status-history|table|table-old|text|timeseries|trend|traces|welcome|xychart|alertmanager|cloudwatch|dashboard|elasticsearch|grafana|grafana-azure-monitor-datasource|graphite|influxdb|jaeger|loki|mixed|mssql|mysql|opentsdb|postgres|prometheus|stackdriver|tempo|testdata|zipkin|phlare|parca)$"

+		// An alias is useful when migrating from one plugin id to another (rebranding etc)
+		// This should be used sparingly, and is currently only supported though a hardcoded checklist
+		alias?: string


I also think it would make sense to just have this as a string[] just for sure. For example if we decide to merge 2 data sources into one. Or if we just make some mistake/typo at some point when renaming.

updated so it is now aliasIds 🎉

Nit. Why not aliases?

github-actions · 2023-09-21T18:38:04Z

⚠️ Possible breaking changes

(Open the links below in a new tab to go to the correct steps)

grafana-data has possible breaking changes (more info)

Console output
Read our guideline

ryantxu · 2023-09-21T18:39:56Z

packages/grafana-data/src/types/plugin.ts

@@ -58,7 +58,7 @@ export interface PluginMeta<T extends KeyValue = {}> {
  info: PluginMetaInfo;
  includes?: PluginInclude[];
  state?: PluginState;
-  alias?: string;
+  aliasIds?: string[];


reports as a breaking change... but I think OK

nit: the camel case for this would be aliasIDs

ryantxu · 2023-09-21T18:40:42Z

pkg/api/frontendsettings.go

@@ -19,6 +19,7 @@ import (
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/tsdb/grafanads"
 	"github.com/grafana/grafana/pkg/util"
+	"golang.org/x/exp/slices"


note this is standard in go 1.21

ryantxu · 2023-09-26T17:57:42Z

@andresmgot -- anything missing to merge this? OK to merge soon, then remove the explicit checks when we know HGAPI won't allow signing plugins with aliasIDs we do not expect?

andresmgot · 2023-09-27T08:59:03Z

pkg/plugins/plugindef/plugindef.cue

@@ -18,6 +18,10 @@ schemas: [{
 		id: string & strings.MinRunes(1)
 		id: =~"^([0-9a-z]+\\-([0-9a-z]+\\-)?(\(strings.Join([ for t in _types {t}], "|"))))|(alertGroups|alertlist|annolist|barchart|bargauge|candlestick|canvas|dashlist|debug|datagrid|gauge|geomap|gettingstarted|graph|heatmap|histogram|icon|live|logs|news|nodeGraph|piechart|pluginlist|stat|state-timeline|status-history|table|table-old|text|timeseries|trend|traces|welcome|xychart|alertmanager|cloudwatch|dashboard|elasticsearch|grafana|grafana-azure-monitor-datasource|graphite|influxdb|jaeger|loki|mixed|mssql|mysql|opentsdb|postgres|prometheus|stackdriver|tempo|grafana-testdata-datasource|zipkin|phlare|parca)$"

+		// An alias is useful when migrating from one plugin id to another (rebranding etc)
+		// This should be used sparingly, and is currently only supported though a hardcoded checklist
+		alias?: [...string]


this needs to be aliasIDs

andresmgot · 2023-09-27T09:01:02Z

packages/grafana-data/src/types/plugin.ts

@@ -58,7 +58,7 @@ export interface PluginMeta<T extends KeyValue = {}> {
  info: PluginMetaInfo;
  includes?: PluginInclude[];
  state?: PluginState;
-  alias?: string;
+  aliasIds?: string[];


nit: the camel case for this would be aliasIDs

andresmgot · 2023-09-27T09:16:58Z

@andresmgot -- anything missing to merge this? OK to merge soon, then remove the explicit checks when we know HGAPI won't allow signing plugins with aliasIDs we do not expect?

I have some minor comments but yes, we can merge once that is addressed and the CI passes

andresmgot

LGTM

marefr

LGTM, but naming it aliases rather than aliasIDs would simplify.

marefr · 2023-09-28T07:28:18Z

pkg/plugins/plugins.go

@@ -75,7 +73,7 @@ type JSONData struct {
 	ID           string       `json:"id"`
 	Type         Type         `json:"type"`
 	Name         string       `json:"name"`
-	Alias        string       `json:"alias,omitempty"`
+	AliasIDs     []string     `json:"aliasIDs,omitempty"`


In json this should probably be camelCase, .e.g. aliasIds. That's why I suggest to name it Aliases/aliases instead to simplify :)

ID is uppercase so the camel case should be aliasIDs?

I also first thought of aliases but I think it's more descriptive to use aliasIDs since it makes it clear that it's expecting plugin IDs, not just any alias like a different name.

for sure we want ID in the name -- the confusion with name/type is already too much :) -- I am personally totally agnostic on the capitalization strategy. In go it is AliasIDs for sure, in json/ts... we use aliasIds elsewhere, but either is fine 🤷🏻

feel free to merge with what you prefer

Yep feel free

* main: (51 commits) Postgres: Make securejson password optional (#75801) Alerting: Manage remote Alertmanager silences (#75452) Replace use of `relref` shortcode with `docs/reference` so links work in Grafana Cloud (#75433) Add canonical for testing new canonical behavior in Grafana Cloud (#75745) InfluxDB SQL: Remove default limitation in the query (#75749) Publish documentation from the HEAD of the matching version branch on tag events (#73809) IDforwarding: forward signed id to plugins (#75651) Navtree: Rename page title from "Infrastructure" to "Integrations" (#75721) Changelog: Updated changelog for 9.5.12 (#75776) Chore: update latest.json to 10.1.4 (#75773) Changelog: Updated changelog for 10.1.4 (#75769) Changelog: Updated changelog for 10.0.8 (#75768) Tempo: TraceQL results as a spans list (#75660) Bug: Exclude 32-bit `arm` builds (#75448) Transformations: Fix inconsistent wording of transformation descriptions (#75746) i18n: dashboard import page (#75664) Fix issue-labeled GitHub Action (#75753) Schema: link table panel to the schema definitions (#75671) Plugins: Move alias support to plugin json (but still hardcoded) (#75129) Sandbox: allow access to window.grafanaBootData for plugins (#75522) ...

alias in plugin json

8ce1061

ryantxu requested review from a team as code owners September 19, 2023 22:12

ryantxu requested review from L-M-K-B, JoaoSilvaGrafana, wbrowne and xnyo and removed request for a team September 19, 2023 22:12

grafana-delivery-bot bot added this to the 10.2.x milestone Sep 19, 2023

ryantxu added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes area/plugins labels Sep 19, 2023

grafana-pr-automation bot added area/frontend area/backend labels Sep 19, 2023

andresmgot reviewed Sep 20, 2023

View reviewed changes

aocenas reviewed Sep 21, 2023

View reviewed changes

allow multiple

9a55b4c

ryantxu requested review from a team as code owners September 21, 2023 18:14

ryantxu requested review from mckn, zserge, mildwonkey and nikimanoledaki and removed request for a team September 21, 2023 18:14

grafana-pr-automation bot added the datasource/Azure Azure Monitor Datasource label Sep 21, 2023

ryantxu added 2 commits September 21, 2023 11:33

merge main

aa1e7c2

merge main

0387e2e

github-actions bot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Sep 21, 2023

ryantxu requested review from andresmgot and tolzhabayev September 21, 2023 18:38

ryantxu commented Sep 21, 2023

View reviewed changes

aangelisc removed their request for review September 21, 2023 19:16

ryantxu added 3 commits September 22, 2023 08:37

merge main

a7172fa

Merge remote-tracking branch 'origin/main' into alias-in-pluginjson2

9cf5a7a

Merge remote-tracking branch 'origin/main' into alias-in-pluginjson2

3d8ce9c

andresmgot reviewed Sep 27, 2023

View reviewed changes

ryantxu added 3 commits September 27, 2023 09:22

Merge remote-tracking branch 'origin/main' into alias-in-pluginjson2

66819e0

aliasIDs

fdd5db8

aliasIDs

d631cc5

andresmgot approved these changes Sep 28, 2023

View reviewed changes

marefr approved these changes Sep 28, 2023

View reviewed changes

merge main

b4c5ea3

ryantxu merged commit 010b246 into main Sep 29, 2023

ryantxu deleted the alias-in-pluginjson2 branch September 29, 2023 15:20

ryantxu mentioned this pull request Oct 7, 2023

Plugins: GetDataSourcesByType does not work with new aliasIds #76154

Closed

zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023

Plugins: Move alias support to plugin json (but still hardcoded) #75129

Plugins: Move alias support to plugin json (but still hardcoded) #75129

Conversation

ryantxu commented Sep 19, 2023 • edited Loading

ryantxu commented Sep 19, 2023

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolzhabayev Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryantxu commented Sep 20, 2023

andresmgot commented Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryantxu commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented Sep 27, 2023

andresmgot left a comment

Choose a reason for hiding this comment

marefr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

ryantxu Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryantxu commented Sep 19, 2023 •

edited

Loading

tolzhabayev Sep 21, 2023 •

edited

Loading

andresmgot Sep 20, 2023 •

edited

Loading

andresmgot commented Sep 21, 2023 •

edited

Loading

github-actions bot commented Sep 21, 2023 •

edited

Loading

andresmgot Sep 28, 2023 •

edited

Loading

ryantxu Sep 28, 2023 •

edited

Loading