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

SQL Datasources: Reinstate SQL data source behavior around database selection when default configured databases already exist #65659

Merged
merged 138 commits into from Jun 6, 2023

Conversation

baldm0mma
Copy link
Contributor

@baldm0mma baldm0mma commented Mar 30, 2023

What is this feature?

This reinstates SQL data source behavior around database selection when default configured databases already exist, or is updated to have one.

Expected behavior for data sources, created either in the user interface or a provisioning file:

  • MySQL/MSSQL
    • no configured database
      • dataset dropdown can choose from all queryable databases
    • configured database
      • dataset dropdown only populates with single configured database
    • configured database is either added or changed
      • panel throws error that there has been a change, advises user to make note of previous query, then let's user update to new database and tables
  • Postgres
    • no configured database
      • panel will throw error notifying user that Postgres NEEDS a default database to function and querying is disabled
    • configured database
      • dataset dropdown only populates with single configured database
    • configured database either added or changed
      • editor and panel throws warning that there has been a change, advises user to make note of previous query, then let's user update to new database and tables

Does not interfere with datasources in Explore mode.

Screenshot 2023-05-17 at 1 23 21 PM (2) Screenshot 2023-05-17 at 1 23 32 PM (2) Screenshot 2023-05-17 at 1 23 46 PM (2)

Why do we need this feature?

To match previous behavior with our sql datasources.

Who is this feature for?

Everyone.

Which issue(s) does this PR fix?:

Fixes #63161 and fixes #64790

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • [na] 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.
  • There are no known compatibility issues with older supported versions of Grafana, or plugins.
  • It passes the Hosted Grafana feature readiness review for observability, scalability, performance, and security.

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/pr-codeql-analysis-javascript.yml:analyze. As part of the setup process, we have scanned this repository and found 27 existing alerts. Please check the repository Security tab to see all alerts.

@baldm0mma baldm0mma changed the title Baldm0mma/sql datasource update SQL Datasources: freeze default database Mar 30, 2023
@baldm0mma baldm0mma marked this pull request as ready for review April 3, 2023 13:00
@baldm0mma baldm0mma requested a review from a team April 3, 2023 13:00
@baldm0mma baldm0mma requested a review from a team as a code owner April 3, 2023 13:00
@baldm0mma baldm0mma requested review from oscarkilhed, zoltanbedi and andresmgot and removed request for a team April 3, 2023 13:00
@baldm0mma baldm0mma added no-backport Skip backport of PR add to changelog labels Apr 3, 2023
@baldm0mma baldm0mma changed the title SQL Datasources: freeze default database SQL Datasources: freeze chosen default database in query editor Apr 3, 2023
@baldm0mma baldm0mma added this to the 9.5.0 milestone Apr 3, 2023
@baldm0mma baldm0mma self-assigned this Apr 3, 2023
Copy link
Contributor

@mdvictor mdvictor left a comment

Choose a reason for hiding this comment

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

It's looking mostly good, left a couple comments and should be good to go!

pkg/api/frontendsettings.go Show resolved Hide resolved
@baldm0mma baldm0mma requested a review from mdvictor May 26, 2023 12:57
@mdvictor
Copy link
Contributor

@baldm0mma This looks great!
Maybe we can also add (either now or in the future) a similar warning somewhere in the query editor, I guess. If you setup, for example, a postgres ds with no default database you'll just see empty select boxes in the editor and only when actually running the query will you get the warning saying that there is no default database set.

The error also seems to be thrown in an unexpected scenario (for me at least). On mssql with set default database, if I skip choosing a dataset and go straight for table and column, I get an invalid query and when running it I get the message that the database configuration has been changed, although it wasn't. e.g:

Screen.Recording.2023-05-29.at.15.13.46.mov

This happens on all 3 ds'es and is a bit confusing. e.g. with all 3:

Screen.Recording.2023-05-29.at.15.16.02.mov

The main problem seems that not selecting the dataset but still setting the other options will trigger that warning, then a user would select a dataset and still have the other selects set but the SQL query preview doesn't show it. Maybe a different warning should be thrown that tells the user to choose a dataset and also after setting the dataset, all other selects should be reset so the user isn't confused seeing the select box set, but the query invalid like in the video

@baldm0mma
Copy link
Contributor Author

baldm0mma commented May 30, 2023

@mdvictor Good catch! This behavior should now be fixed. I think you're right about adding a similar warning in the query editor, but I think that should be in another PR; there's a number of similar, small updates I'd like to make like the one you mentioned, but that don't really belong in this PR.

@baldm0mma This looks great! Maybe we can also add (either now or in the future) a similar warning somewhere in the query editor, I guess. If you setup, for example, a postgres ds with no default database you'll just see empty select boxes in the editor and only when actually running the query will you get the warning saying that there is no default database set.

The error also seems to be thrown in an unexpected scenario (for me at least). On mssql with set default database, if I skip choosing a dataset and go straight for table and column, I get an invalid query and when running it I get the message that the database configuration has been changed, although it wasn't. e.g:

Screen.Recording.2023-05-29.at.15.13.46.mov
This happens on all 3 ds'es and is a bit confusing. e.g. with all 3:

Screen.Recording.2023-05-29.at.15.16.02.mov
The main problem seems that not selecting the dataset but still setting the other options will trigger that warning, then a user would select a dataset and still have the other selects set but the SQL query preview doesn't show it. Maybe a different warning should be thrown that tells the user to choose a dataset and also after setting the dataset, all other selects should be reset so the user isn't confused seeing the select box set, but the query invalid like in the video

@baldm0mma baldm0mma requested a review from mdvictor May 30, 2023 17:42
preconfiguredDataset,
}: DatasetSelectorProps) => {
/*
The behavior of this component - for MSSQL and MySQL datasources - is based on whether the user chose to create a datasource
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome work with the explanations!

Copy link
Contributor

@mdvictor mdvictor left a comment

Choose a reason for hiding this comment

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

LGTM! Great work with this one!

@baldm0mma baldm0mma requested a review from a team as a code owner June 6, 2023 16:15
@baldm0mma baldm0mma merged commit c0a1fc2 into main Jun 6, 2023
17 checks passed
@baldm0mma baldm0mma deleted the baldm0mma/sql_datasource_update branch June 6, 2023 16:28
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
@vchirikov
Copy link

10.1.0 - Closed 2 weeks ago 100% complete

@ricky-undeadcoders Could you tell us when the docker image with this feature will be available?

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