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

FrameMeta: add PreferredVisualizationPluginId field to API #705

Merged
merged 2 commits into from
Jul 7, 2023

Conversation

Umaaz
Copy link
Contributor

@Umaaz Umaaz commented Jun 20, 2023

What this PR does / why we need it:
This will add the field '' to the FrameMeta type. This is linked to the PR I have open on grafana/grafana grafana/grafana#66982.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@CLAassistant
Copy link

CLAassistant commented Jun 20, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Umaaz Umaaz marked this pull request as ready for review July 5, 2023 09:53
@Umaaz Umaaz requested a review from a team as a code owner July 5, 2023 09:53
@ifrost ifrost self-requested a review July 5, 2023 11:51
Copy link
Contributor

@yesoreyeram yesoreyeram left a comment

Choose a reason for hiding this comment

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

LGTM. @ifrost - does this mean, are we deprecating preferredVisualisationType?

Also can we have set of typed aliases for common/mostly used plugin ids such as timeseries, stat etc to avoid typos in future?

type PluginId string

const (
  PluginIdTimeSeriesPanel PluginId = "timeseries"
  PluginIdStatPanel PluginId = "stat"
)

/// and then use as

PreferredVisualizationPluginID PluginId `json:"preferredVisualisationPluginId,omitempty"`

@ifrost
Copy link

ifrost commented Jul 7, 2023

does this mean, are we deprecating preferredVisualisationType?

No, preferredVisualisationType is a more generic type, while preferredVisualisationPluginId is used to override the visualization type to a specific plugin. It's marked as alpha, we will see if we need both properties in the future.

Also can we have set of typed aliases for common/mostly used plugin ids such as timeseries, stat etc to avoid typos in furture?

Sounds like a good idea. I'm not sure how to guarantee that such an enum would stay up to date in case something changes or is being added, though. Maybe there should be a schema for it that would generate ts and go types?

@yesoreyeram
Copy link
Contributor

yesoreyeram commented Jul 7, 2023

I'm not sure how to guarantee that such an enum would stay up to date in case something changes or is being added, though. Maybe there should be a schema for it that would generate ts and go types?

Usually we don't change plugin ids. So this won't be a problem. Also this accepts string as a fallback. ( for community plugins, string should work as a fallback ). Also this enum approach doesn't guarantee but adding one level of protection and adding convenience.

Just an idea but not necessarily to go with this PR.

@ifrost ifrost merged commit 6a36850 into grafana:main Jul 7, 2023
2 checks passed
@tolzhabayev
Copy link
Contributor

We do not change plugin ids per se but we want to introduce aliases grafana/grafana#69874 and maybe UIDs for plugins internally as heads up to @yesoreyeram and @ifrost (fyi @wbrowne )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants