-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
refactor(sqllab): migrate share queries via kv by permalink #29163
base: master
Are you sure you want to change the base?
refactor(sqllab): migrate share queries via kv by permalink #29163
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #29163 +/- ##
==========================================
+ Coverage 60.48% 70.47% +9.98%
==========================================
Files 1931 1960 +29
Lines 76236 77838 +1602
Branches 8568 8765 +197
==========================================
+ Hits 46114 54855 +8741
+ Misses 28017 20823 -7194
- Partials 2105 2160 +55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/testenv up |
@villebro Ephemeral environment spinning up at http://52.37.81.73:8080. Credentials are |
/testenv up FEATURE_SHARE_QUERIES_VIA_KV_STORE=true |
@villebro Ephemeral environment spinning up at http://35.86.167.107:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvement, this is long overdue! I suggest removing the SHARE_QUERIES_VIA_KV_STORE
feature flag, and making the permalink only store the selected portion of the editor. Other than that this looks great!
if (isFeatureEnabled(FeatureFlag.ShareQueriesViaKvStore)) { | ||
return getCopyUrlForKvStore(callback); | ||
return getCopyUrlForPermalink(callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this feature be enabled by default? Now the permalink feature is only available if the SHARE_QUERIES_VIA_KV_STORE
feature flag is enabled:
I feel we could now remove the feature flag, and make this the default behavior.
In addition, I think it would be a good idea to create the permalink based on the same query snippet that gets executed when running the query - Currently the entire editor gets persisted into the permalink, even if only a specific portion of the editor is selected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we could now remove the feature flag, and make this the default behavior.
@villebro as @michael-s-molina mentioned, we'll remove the feature flag during the 5.0 proposal work.
In addition, I think it would be a good idea to create the permalink based on the same query snippet that gets executed when running the query - Currently the entire editor gets persisted into the permalink, even if only a specific portion of the editor is selected.
Clipping only specific executed sections into a permalink might cause confusion for some users. Instead, I'm thinking about a method where, if an area is selected, it automatically highlights the selection in the editor.
@villebro @michael-s-molina could you take a look? |
Can you add support for old saved query URLs? |
SUMMARY
This PR changes the existing
SHARE_QUERIES_VIA_KV_STORE
feature to store data using the permalink API instead of the KV store. This is one of the prerequisites of Remove old KV store endpoint and its associated asset.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
after--permalink.mov
TESTING INSTRUCTIONS
Go to SQL Lab and then click "Copy Link"
ADDITIONAL INFORMATION