feat: import/export dashboards via cli #5991
Conversation
|
@mistercrunch I am not sure why |
|
That test is flaky. Please try to re-run. @graceguo-supercat and I are working to make it more stable. Sorry for the inconvenience.
…On Sep 27, 2018, 23:42 -0700, Arpit ***@***.***>, wrote:
@mistercrunch I am not sure why cypress test is failing as there is no change related to that.
On my dev machine it is working fine.
Please validate if any recent merge to master may causing this.
@kristw This is related recent commit on master. can you help me in fixing cypress build here
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Thanks for update @kristw. This is open for review. I will commit testcase and docstring docs in couple of days. |
|
@mistercrunch @kristw The build is failing for unrelated test on async query celery. Can you help me fix that @mistercrunch I saw this job also failed for same test case couple of days ago. |
Import is a wrapper around exist functionality so we can go ahead without a test or mock the actual db operation using https://docs.python.org/3/library/unittest.mock.html And validate the wrapper operations only.
arpit-agarwal
force-pushed
the
feat/dashboard_import_export_cli
branch
from
30a8d04 to
f5922f5
Compare
Codecov Report
@@ Coverage Diff @@
## master #5991 +/- ##
==========================================
+ Coverage 63.44% 63.45% +0.01%
==========================================
Files 443 444 +1
Lines 23753 23803 +50
Branches 2638 2638
==========================================
+ Hits 15070 15105 +35
- Misses 8670 8685 +15
Partials 13 13
Continue to review full report at Codecov.
|
| @@ -547,6 +550,34 @@ def test_import_druid_override_identical(self): | |||
| self.assert_datasource_equals( | |||
| copy_datasource, self.get_datasource(imported_id)) | |||
|
|
|||
| def test_export_dashboards_util(self): | |||
There was a problem hiding this comment.
@mistercrunch @arpit-agarwal I've noticed this test failing the py36-postgres tests when a pr has not made any changes to this code. For example https://travis-ci.org/apache/incubator-superset/jobs/436740957 Can you take a look?
There was a problem hiding this comment.
@mistercrunch @arpit-agarwal this is blocking merging all PRs into master
There was a problem hiding this comment.
@mistercrunch @arpit-agarwal this flaky test is still failing most builds for py36-postgres therefore blocking merges. Can you either PTAL or we will revert this PR if it's not fixed by noon today PDT?
There was a problem hiding this comment.
i made a test branch which reverted this whole PR, and it passed all CI tests. So i created #6035 to revert this. Thank you!
There was a problem hiding this comment.
Thank guys.
@graceguo-supercat @williaster @mistercrunch
However I don't understand how this PR can halt postgres.
I had seen stalled postgres even before this PR was merged. Look this job for a instance that was commit before this PR merged.
I also see build not passing after reverting the PR. see
We may need to find the first stalled build to see the root cause. I am out in a pycon India i will grok the logs once back
There was a problem hiding this comment.
@arpit-agarwal Thank you very much for investigation. After I reverted this PR, what we observed is, most PRs passed all CI tests, includes postgres one. But master branch still failed at postgres test. Right now i am totally confused what exactly happened :(
There was a problem hiding this comment.
As suggested we can look at the content of PR #5693 to fund the route cause.
I do see some changes in test in that PR related to test.
Any recent configruration changes on tarvis or postgress version or lsome python lib?
|
this PR is now reverted. see #6035. |
|
I quickly looked at logs it seems this is the first failure point introduced by PR #5693 |
|
I see master build is passing now. did we made some changes in configuration to fix it? If all is going good can we raise The pR again to merge this feature. If we are skeptical about test we can have a separate PR for test? |
|
Yes i made some change from our travis setting. Sorry I reverted your PR and now think your PR should be good 👍. Do you want to create a new PR? i will approve it and merge. |
|
Yes. I will create a new PR shortly |
|
@graceguo-supercat I have raised #6061 with these changes. It will be great if we can document the travis changes for future reference. |
* feat: import/export dashboards via cli * style: fixed lint error * test: added test for import and export util * test: removing import test as it is causing integrity issues Import is a wrapper around exist functionality so we can go ahead without a test or mock the actual db operation using https://docs.python.org/3/library/unittest.mock.html And validate the wrapper operations only. * test: remove test data file * test: removed usage of reserved keyword id
mistercrunch
added
🏷️ bot
This PR is initial implementation to close #5821
@mistercrunch
Please validate correctness of implementation
I will add documentation and test in couple of days.