-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
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% 83.45% +22.97%
===========================================
Files 1931 546 -1385
Lines 76236 39249 -36987
Branches 8568 0 -8568
===========================================
- Hits 46114 32757 -13357
+ Misses 28017 6492 -21525
+ Partials 2105 0 -2105
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!
superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx
Outdated
Show resolved
Hide resolved
@villebro @michael-s-molina could you take a look? |
Can you add support for old saved query URLs? |
26036ce
to
a55adb7
Compare
Done. @michael-s-molina PTAL |
f1afa3e
to
004f52b
Compare
@@ -62,6 +62,7 @@ These features flags are **safe for production**. They have been tested and will | |||
[//]: # "PLEASE KEEP THESE LISTS SORTED ALPHABETICALLY" | |||
|
|||
### Flags on the path to feature launch and flag deprecation/removal | |||
|
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.
Did you forget to remove the SHARE_QUERIES_VIA_KV_STORE
and KV_STORE
from this document?
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.
LGTM. Thanks for addressing the comments @justinpark!
8912670
to
45e570b
Compare
Hey, @justinpark I just fetched this to local and when I try to copy the link in Sqllab i get the error, "Request URL: |
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--permalink2.mov
TESTING INSTRUCTIONS
Go to SQL Lab and then click "Copy Link"
ADDITIONAL INFORMATION