You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/02/11 19:30:19 UTC
[GitHub] [incubator-superset] willbarrett opened a new pull request #9120:
Add feature flags to control query sharing, KV exposure
willbarrett opened a new pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120
### CATEGORY
Choose one
- [ ] Bug Fix
- [X] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Preset would like query sharing to abide by the rules of query visibility used in the rest of the project. To accomplish this, we have updated the `Share Query` button to leverage the saved query functionality rather than the `KV` model, with a feature flag should other organizations want to maintain the current functionality. We have also added a feature flag to disable the `/kv` endpoints entirely. They appear to be used only for query sharing inside of the Superset project, but we do not know how these are leveraged in other systems.
Note that following the link to the saved query will *open* the saved query in SQLLab, rather than the current behavior, which is to *open a copy* of the saved query in SQLLab.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
"Save Query" toast after the change:
With an unsaved query:
<img width="555" alt="Screen Shot 2020-02-11 at 11 27 04 AM" src="https://user-images.githubusercontent.com/1779034/74271383-a93d7e00-4cc1-11ea-8fce-74c5fc06e9c9.png">
With a saved query:
<img width="544" alt="Screen Shot 2020-02-11 at 11 26 52 AM" src="https://user-images.githubusercontent.com/1779034/74271405-b0fd2280-4cc1-11ea-88c7-00379ab80631.png">
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [X] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [X] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
TBD
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9120: Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#discussion_r377910820
##########
File path: superset-frontend/src/SqlLab/components/ShareSqlLabQuery.jsx
##########
@@ -45,9 +47,19 @@ class ShareSqlLabQuery extends React.Component {
shortUrl: t('Loading ...'),
};
this.getCopyUrl = this.getCopyUrl.bind(this);
+ this.getCopyUrlForSavedQuery = this.getCopyUrlForSavedQuery.bind(this);
+ this.getCopyUrlForKvStore = this.getCopyUrlForKvStore.bind(this);
Review comment:
`.bind(this)` is only necessary when you are going to be passing `this.getCopyUrlForKvStore` around as a separate variable. If it's only called via `this.getCopyUrlForKvStore()` then `this` will be bound correctly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9120: Add feature
flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#issuecomment-585858397
The summary of the conversation was to add some documentation about how to reenable the feature if desired and update the UPDATING.md file
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9120: Add
feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#issuecomment-586410483
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=h1) Report
> Merging [#9120](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/291306392443a5a0d0e2ee0cc4a95d37c56d4589?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `81.81%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9120 +/- ##
==========================================
- Coverage 59.1% 59.08% -0.02%
==========================================
Files 372 372
Lines 11920 11933 +13
Branches 2917 2921 +4
==========================================
+ Hits 7045 7051 +6
- Misses 4693 4700 +7
Partials 182 182
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/featureFlags.ts](https://codecov.io/gh/apache/incubator-superset/pull/9120/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZlYXR1cmVGbGFncy50cw==) | `91.66% <100%> (+0.75%)` | :arrow_up: |
| [...rontend/src/SqlLab/components/ShareSqlLabQuery.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9120/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NoYXJlU3FsTGFiUXVlcnkuanN4) | `93.54% <80%> (-6.46%)` | :arrow_down: |
| [superset-frontend/src/components/Loading.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9120/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTG9hZGluZy5qc3g=) | `55.55% <0%> (-44.45%)` | :arrow_down: |
| [...frontend/src/dashboard/components/MissingChart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9120/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL01pc3NpbmdDaGFydC5qc3g=) | `100% <0%> (ø)` | :arrow_up: |
| [superset-frontend/src/chart/chartAction.js](https://codecov.io/gh/apache/incubator-superset/pull/9120/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L2NoYXJ0QWN0aW9uLmpz) | `43.33% <0%> (+0.09%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=footer). Last update [2913063...ea37902](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9120: Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#discussion_r380608751
##########
File path: tests/core_tests.py
##########
@@ -497,6 +504,9 @@ def test_shortner(self):
resp = self.client.post("/r/shortner/", data=dict(data=data))
assert re.search(r"\/r\/[0-9]+", resp.data.decode("utf-8"))
+ @skipUnless(
Review comment:
Could enabling this feature on just for the tests be a viable approach? `superset_test_config.py`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9120: Add feature
flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#issuecomment-585460932
This seems like a pretty major product/functionality change as far as completely removing the concept of one-off query sharing (i'm pretty sure the kv endpoints are also used for one-off chart sharing too). What tradeoffs were considered when thinking about this feature removal? I personally make use of the query and chart sharing features all the time.
cc @sylvia-tomiyama for her take too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9120: Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#discussion_r377910820
##########
File path: superset-frontend/src/SqlLab/components/ShareSqlLabQuery.jsx
##########
@@ -45,9 +47,19 @@ class ShareSqlLabQuery extends React.Component {
shortUrl: t('Loading ...'),
};
this.getCopyUrl = this.getCopyUrl.bind(this);
+ this.getCopyUrlForSavedQuery = this.getCopyUrlForSavedQuery.bind(this);
+ this.getCopyUrlForKvStore = this.getCopyUrlForKvStore.bind(this);
Review comment:
`.bind(this)` is only necessary when you are going to be passing `this.getCopyUrlForKvStore` around as a separate variable. If it's only called via `this.getCopyUrlForKvStore` then `this` will be bound correctly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9120: Add feature
flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#issuecomment-585462901
Could you expand a bit on the insecure nature of the `/kv` endpoints? It was my understanding that they'd let you view a query/chart form data, but actually running the query would still be dependent on the security manager granting you access to the base level datasources. They seem not unlike the google doc link sharing feature, and if the predictable nature of the urls is an issue then simply using a random key would resolve that issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9120: Add
feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#issuecomment-585461820
@etr2460 the unsecured/unvalidated/unowned `/kv/` endpoints are unacceptable from a security standpoint to Preset. We've provided feature flags so that other organizations can continue using the system as-is. The Cartel designs have a different model entirely for sharing, so we view this fix as a stop-gap in advance of that work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9120: Add
feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#issuecomment-585464421
@etr2460 I'll expand directly with you in Slack. I don't wish to have a detailed security-related conversation publicly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9120: Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#discussion_r380822437
##########
File path: tests/core_tests.py
##########
@@ -497,6 +504,9 @@ def test_shortner(self):
resp = self.client.post("/r/shortner/", data=dict(data=data))
assert re.search(r"\/r\/[0-9]+", resp.data.decode("utf-8"))
+ @skipUnless(
Review comment:
In this case it would be, since the feature flag only removes functionality and doesn't add anything different. that said, I think we should be able to test both sides of the feature flag here, do you know how to do this @dpgaspar ?
Maybe mocking the feature flag checking function?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9120:
Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#issuecomment-586410483
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@e5c7adf`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `50%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9120 +/- ##
=========================================
Coverage ? 59.08%
=========================================
Files ? 372
Lines ? 11933
Branches ? 2921
=========================================
Hits ? 7051
Misses ? 4700
Partials ? 182
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...frontend/src/dashboard/components/MissingChart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9120/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL01pc3NpbmdDaGFydC5qc3g=) | `100% <ø> (ø)` | |
| [superset-frontend/src/chart/chartAction.js](https://codecov.io/gh/apache/incubator-superset/pull/9120/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L2NoYXJ0QWN0aW9uLmpz) | `43.33% <50%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=footer). Last update [e5c7adf...dbe24cb](https://codecov.io/gh/apache/incubator-superset/pull/9120?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9120: Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#discussion_r380916207
##########
File path: tests/core_tests.py
##########
@@ -497,6 +504,9 @@ def test_shortner(self):
resp = self.client.post("/r/shortner/", data=dict(data=data))
assert re.search(r"\/r\/[0-9]+", resp.data.decode("utf-8"))
+ @skipUnless(
Review comment:
The issue in this case is that the application is booted once for the test suite and the feature flag changes the nature of the instantiation. This is a boot-time flag, not a run-time check. For this PR I'll go ahead and enable it for tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9120: Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#discussion_r377923525
##########
File path: superset-frontend/src/SqlLab/components/ShareSqlLabQuery.jsx
##########
@@ -45,9 +47,19 @@ class ShareSqlLabQuery extends React.Component {
shortUrl: t('Loading ...'),
};
this.getCopyUrl = this.getCopyUrl.bind(this);
+ this.getCopyUrlForSavedQuery = this.getCopyUrlForSavedQuery.bind(this);
+ this.getCopyUrlForKvStore = this.getCopyUrlForKvStore.bind(this);
Review comment:
I'll try removing them - I had to add them at one point to get things functioning.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9120: Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#discussion_r379551238
##########
File path: tests/core_tests.py
##########
@@ -497,6 +504,9 @@ def test_shortner(self):
resp = self.client.post("/r/shortner/", data=dict(data=data))
assert re.search(r"\/r\/[0-9]+", resp.data.decode("utf-8"))
+ @skipUnless(
Review comment:
@etr2460 I spent an hour banging my head against it, and was unable to find an approach to modify feature flags after they were initialized without breaking the whole suite. I'm very open to recommendations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9120: Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#discussion_r379175710
##########
File path: UPDATING.md
##########
@@ -22,6 +22,12 @@ This file documents any backwards-incompatible changes in Superset and
assists people when migrating to a new version.
## Next
+* [9120](https://github.com/apache/incubator-superset/pull/9120): Changes the default behavior of ad-hoc sharing of
+queries in SQLLab to one that links to the saved query rather than one that copies the query data into the KVStore
+model and links to the record there. This is a security-related change that makes SQLLab query
+sharing respect the existing RBAC. Should you wish to retain the existing behavior, set two feature flags:
Review comment:
i'd recommend using the full wording instead of RBAC as an acronym
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9120: Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#discussion_r379209119
##########
File path: tests/core_tests.py
##########
@@ -497,6 +504,9 @@ def test_shortner(self):
resp = self.client.post("/r/shortner/", data=dict(data=data))
assert re.search(r"\/r\/[0-9]+", resp.data.decode("utf-8"))
+ @skipUnless(
Review comment:
These should always be run in our test environment whether the flag is enabled or disabled. Can you set the feature flag to on before running them?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9120: Add
feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#issuecomment-586015168
@etr2460 let me know if that entry in UPDATING.md fits your needs or if there's somewhere else you'd like me to drop a note as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda merged pull request #9120: Add
feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
craig-rueda merged pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9120: Add feature flags to control query sharing, KV exposure
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9120: Add feature flags to control query sharing, KV exposure
URL: https://github.com/apache/incubator-superset/pull/9120#discussion_r377910820
##########
File path: superset-frontend/src/SqlLab/components/ShareSqlLabQuery.jsx
##########
@@ -45,9 +47,19 @@ class ShareSqlLabQuery extends React.Component {
shortUrl: t('Loading ...'),
};
this.getCopyUrl = this.getCopyUrl.bind(this);
+ this.getCopyUrlForSavedQuery = this.getCopyUrlForSavedQuery.bind(this);
+ this.getCopyUrlForKvStore = this.getCopyUrlForKvStore.bind(this);
Review comment:
`.bind(this)` is only necessary when you are going to be passing `this.getCopyUrlForKvStore` around as a separate variable. If it's only called via `this.getCopyUrlForKvStore()` then `this` will be bound correctly and these lines are unnecessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org