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

Scopes: Adapt for new API #87841

Merged
merged 6 commits into from
May 15, 2024
Merged

Scopes: Adapt for new API #87841

merged 6 commits into from
May 15, 2024

Conversation

bfmatei
Copy link
Contributor

@bfmatei bfmatei commented May 14, 2024

What is this feature?

The Scopes API moved to a node-based approach. This PR brings compatibility for the UI to use that client.

Why do we need this feature?

Implement Scopes?

Who is this feature for?

Which issue(s) does this PR fix?:

Special notes for your reviewer:

  • Video:
Screen.Recording.2024-05-16.at.11.54.39.mov

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.

@bfmatei bfmatei requested review from a team as code owners May 14, 2024 15:24
@bfmatei bfmatei requested review from axelavargas, Clarity-89, L-M-K-B and leventebalogh and removed request for a team May 14, 2024 15:24
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone May 14, 2024
@bfmatei bfmatei self-assigned this May 14, 2024
@bfmatei bfmatei added no-changelog Skip including change in changelog/release notes area/scopes scopes feature labels May 14, 2024
@bfmatei bfmatei added team/grafana-dashboards Dashboards squad and removed area/frontend labels May 14, 2024
@grafanabot
Copy link
Contributor

❌ Failed to run Playwright plugin e2e tests.

Click here to browse the Playwright report and trace viewer.
For information on how to run Playwright tests locally, refer to the Developer guide.

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.

@bfmatei - the overall flow works good I think, and API adjustments are fine.

However, I have concerns about the UX, on top of the comments that I've left:

  1. Any change in the scope selector (selecting a scope) results in new queries. I think we may want to defer that until the scopes selector is closed.
  2. Would love the entire scope selector row to be clickable rather than the checkbox only.
  3. I miss some placeholder on the empty scope selector, currently it's just an empty input.

Would be also good if we added some tests for the tree navigation UI to make sure we are not breaking anything in the future.

@grafana-pr-automation
Copy link

grafana-pr-automation bot commented May 15, 2024

⚠️   Possible breaking changes (md version)   ⚠️

grafana-data

Removals

DataQueryRequest.scope
/home/runner/work/grafana/grafana/base/grafana-data/dist/index.d.ts
scope?: Scope | undefined;

Changes

CustomVariableSupport.query
/home/runner/work/grafana/grafana/pr/grafana-data/dist/index.d.ts
Parameter type changed:
     endTime?: number;
     liveStreaming?: boolean;
     hideFromInspector?: boolean;
     queryGroupId?: string;
-    scope?: Scope | undefined;
+    scopes?: Scope[] | undefined;
 }


DataSourceApi.query

/home/runner/work/grafana/grafana/pr/grafana-data/dist/index.d.ts

Parameter type changed:
     endTime?: number;
     liveStreaming?: boolean;
     hideFromInspector?: boolean;
     queryGroupId?: string;
-    scope?: Scope | undefined;
+    scopes?: Scope[] | undefined;
 }

DataSourceWithSupplementaryQueriesSupport.getDataProvider

/home/runner/work/grafana/grafana/pr/grafana-data/dist/index.d.ts

Parameter type changed:
     endTime?: number;
     liveStreaming?: boolean;
     hideFromInspector?: boolean;
     queryGroupId?: string;
-    scope?: Scope | undefined;
+    scopes?: Scope[] | undefined;
 }

DataSourceWithSupplementaryQueriesSupport.getSupplementaryRequest

/home/runner/work/grafana/grafana/pr/grafana-data/dist/index.d.ts

Parameter type changed:
     endTime?: number;
     liveStreaming?: boolean;
     hideFromInspector?: boolean;
     queryGroupId?: string;
-    scope?: Scope | undefined;
+    scopes?: Scope[] | undefined;
 }

StandardVariableSupport.query

/home/runner/work/grafana/grafana/pr/grafana-data/dist/index.d.ts

Parameter type changed:
     endTime?: number;
     liveStreaming?: boolean;
     hideFromInspector?: boolean;
     queryGroupId?: string;
-    scope?: Scope | undefined;
+    scopes?: Scope[] | undefined;
 }


grafana-prometheus

Removals

Prometheus.scope

/home/runner/work/grafana/grafana/base/grafana-prometheus/dist/index.d.ts

scope?: ScopeSpec;


Read our guideline
Console output

@grafana-pr-automation grafana-pr-automation bot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label May 15, 2024
@grafana-pr-automation grafana-pr-automation bot requested a review from a team May 15, 2024 09:36
@bfmatei bfmatei merged commit 6127dfd into main May 15, 2024
17 of 18 checks passed
@bfmatei bfmatei deleted the bogdan/scopes-new-api branch May 15, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend area/frontend area/scopes scopes feature levitate breaking change A label indicating a breaking change and assigned by Levitate. no-changelog Skip including change in changelog/release notes team/grafana-dashboards Dashboards squad
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants