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 2022/04/07 16:44:31 UTC
[GitHub] [superset] pnadolny13 opened a new pull request, #19595: fix: Volatile datasource ordering in dashboard export
pnadolny13 opened a new pull request, #19595:
URL: https://github.com/apache/superset/pull/19595
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
When using the export_dashboards command on the CLI the order of the datasources in the json is inconsistent so if you're trying to use git versioning theres a big diff from reordering. This PR implements sorting so this no longer happens.
Resolves: https://github.com/apache/superset/issues/19478
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
NA
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
I edited https://github.com/apache/superset/blob/f91f83d011b4c83576f5fff76d502f5616f2972d/tests/integration_tests/cli_tests.py#L76 like:
```python
json_content = json.loads(contents)
# check that datasources are ordered consistently
assert json_content.get("datasources")[0].get("__SqlaTable__").get("database_id") == 1
```
to assert the order of the datasource ids but none of the existing dashboard fixtures have multiple datasources and I couldnt figure out how to add one with multiple datasources.
To manually test prior to this PR:
1. Create multiple datasources in your Superset instance
2. Create a chart with each datasource
3. Add both charts to a dashboard
4. Export the dashboard using the CLI
5. Make a copy of the exported file
6. Export again and do a diff on the new vs copied file (you might need to export a few times before it changes because the ordering is random)
Following this PR there should never be a diff in the datasources if nothing changed.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Volatile datasource ordering in dashboard export [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #19595:
URL: https://github.com/apache/superset/pull/19595#issuecomment-1962011441
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/19595?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `3 lines` in your changes are missing coverage. Please review.
> Project coverage is 59.63%. Comparing base [(`11760d3`)](https://app.codecov.io/gh/apache/superset/commit/11760d3fbf683e10ecbf2c9161248697c1acb6fc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`c167c8e`)](https://app.codecov.io/gh/apache/superset/pull/19595?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files](https://app.codecov.io/gh/apache/superset/pull/19595?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/models/dashboard.py](https://app.codecov.io/gh/apache/superset/pull/19595?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL2Rhc2hib2FyZC5weQ==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/19595?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19595 +/- ##
==========================================
- Coverage 69.56% 59.63% -9.93%
==========================================
Files 1908 1908
Lines 74519 74521 +2
Branches 8309 8309
==========================================
- Hits 51839 44442 -7397
- Misses 20627 28026 +7399
Partials 2053 2053
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/19595/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [mysql](https://app.codecov.io/gh/apache/superset/pull/19595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/19595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/19595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.74% <0.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/19595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.27% <0.00%> (-20.60%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/19595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/19595/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.46% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/19595?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Volatile datasource ordering in dashboard export [superset]
Posted by "mdeshmu (via GitHub)" <gi...@apache.org>.
mdeshmu commented on PR #19595:
URL: https://github.com/apache/superset/pull/19595#issuecomment-1996904750
Any update on this?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Volatile datasource ordering in dashboard export [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #19595:
URL: https://github.com/apache/superset/pull/19595#issuecomment-1930682547
@pnadolny13 are you able to rebase this PR so it's mergeable, and we can get this across the finish line?
CC @betodealmeida in case anything has changed in this realm since the PR was opened.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Volatile datasource ordering in dashboard export [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #19595:
URL: https://github.com/apache/superset/pull/19595#issuecomment-1962007371
Rebased
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Volatile datasource ordering in dashboard export [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #19595:
URL: https://github.com/apache/superset/pull/19595
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Volatile datasource ordering in dashboard export [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #19595:
URL: https://github.com/apache/superset/pull/19595#issuecomment-1997826632
> Can you think of any concerns here @michael-s-molina?
No concerns. I was just wondering if the same ordering problem can happen with other entities. If that's the case, maybe a solution that account for all entities, and not only datasources, would be better to deal with `git diff `problems. @betodealmeida probably has more context than me to evaluate the proposal.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org