Skip to content
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

fix: dashboard endpoint sig changed #10220

Merged
merged 4 commits into from Jul 9, 2020

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Jul 1, 2020

SUMMARY

/superset/dashboard/<id> Flask endpoint signature changed on #9939.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@dpgaspar dpgaspar marked this pull request as ready for review July 1, 2020 17:47
Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, and apologies for the regression.

@willbarrett
Copy link
Member

Can we add a unit test to avoid future regressions?

@pull-request-size pull-request-size bot added size/S and removed size/XS labels Jul 9, 2020
@dpgaspar dpgaspar requested a review from villebro July 9, 2020 10:17
@dpgaspar dpgaspar changed the title fix(thumbnails): dashboard endpoint sig changed fix: dashboard endpoint sig changed Jul 9, 2020
@dpgaspar dpgaspar added the v0.37 label Jul 9, 2020
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! It seems slightly brittle to accept any kwarg, as linters aren't able to pick up on which are valid and which aren't. Perhaps out of scope for this PR, but to avoid future regressions, perhaps we should have named arguments for the valid ones, and those then get pushed into a dict that is fed to url_for(**kwargs) in get_url_for_path.

@dpgaspar
Copy link
Member Author

dpgaspar commented Jul 9, 2020

Good point, I'll merge this and make a subsequent PR

@dpgaspar dpgaspar merged commit 6224edd into apache:master Jul 9, 2020
@dpgaspar dpgaspar deleted the fix/thumbnails-regression branch July 9, 2020 10:42
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* fix(thumbnails): dashboard endpoint sig changed

* fix, flask get url for Superset.dashboard

* add simple test
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S v0.37 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants