Skip to content

feat: Add Parameters fields to GET Database#14653

Merged
hughhhh merged 29 commits into
masterfrom
add-parameters-api
May 19, 2021
Merged

feat: Add Parameters fields to GET Database#14653
hughhhh merged 29 commits into
masterfrom
add-parameters-api

Conversation

@hughhhh
Copy link
Copy Markdown
Member

@hughhhh hughhhh commented May 16, 2021

SUMMARY

Update the api to surface parameters in GET database. This will be leveraged for whenever a user wants to edit a database assuming configuration type is dynamic.

Also, updated all engine specs to allow for abstract classes while we implement them.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@codecov
Copy link
Copy Markdown

codecov Bot commented May 16, 2021

Codecov Report

Merging #14653 (b903411) into master (32f5f36) will decrease coverage by 0.21%.
The diff coverage is 66.66%.

❗ Current head b903411 differs from pull request most recent head ef0f9a8. Consider uploading reports for the commit ef0f9a8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14653      +/-   ##
==========================================
- Coverage   77.51%   77.30%   -0.22%     
==========================================
  Files         958      958              
  Lines       48560    48598      +38     
  Branches     5703     5715      +12     
==========================================
- Hits        37642    37568      -74     
- Misses      10718    10829     +111     
- Partials      200      201       +1     
Flag Coverage Δ
hive ?
javascript 72.49% <60.97%> (-0.04%) ⬇️
presto ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...-frontend/src/SqlLab/components/SouthPane/state.ts 100.00% <ø> (ø)
...erset-frontend/src/SqlLab/components/SqlEditor.jsx 54.62% <ø> (ø)
...src/components/FilterableTable/FilterableTable.tsx 82.26% <ø> (ø)
superset/databases/api.py 92.41% <ø> (ø)
superset/db_engine_specs/base.py 87.90% <50.00%> (-0.56%) ⬇️
...erset-frontend/src/SqlLab/components/ResultSet.tsx 67.31% <57.89%> (-2.69%) ⬇️
...rset-frontend/src/SqlLab/components/QueryTable.jsx 67.85% <100.00%> (+1.19%) ⬆️
...tend/src/SqlLab/components/SouthPane/SouthPane.tsx 79.54% <100.00%> (ø)
superset/models/core.py 89.70% <100.00%> (-0.08%) ⬇️
superset/views/core.py 75.48% <100.00%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7a011c...ef0f9a8. Read the comment docs.

@pull-request-size pull-request-size Bot added size/S and removed size/M labels May 16, 2021
@pkdotson
Copy link
Copy Markdown
Member

/testenv up

@github-actions
Copy link
Copy Markdown
Contributor

@pkdotson Ephemeral environment spinning up at http://54.188.67.191:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.


// should show error alerts
cy.get('.toast').contains('error').should('be.visible');
// TODO(hugh): Update this test
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We have a clubhouse ticket to re write this test once we are done with updating the modal

@betodealmeida betodealmeida self-requested a review May 17, 2021 22:09
Comment thread superset/databases/schemas.py Outdated
Comment on lines +251 to +252
if len(parameters.keys()) < 2:
return data
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need this?

Comment thread superset/models/core.py Outdated
hughhhh and others added 3 commits May 17, 2021 21:11
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
@pull-request-size pull-request-size Bot added size/M and removed size/S labels May 18, 2021
@pull-request-size pull-request-size Bot added size/L and removed size/M labels May 18, 2021
@pull-request-size pull-request-size Bot added size/M and removed size/L labels May 18, 2021
Comment thread superset/db_engine_specs/base.py Outdated
Comment on lines +1312 to +1313
def build_sqlalchemy_url(cls, parameters: Any) -> str:
raise NotImplementedError("build_sqlalchemy_url is not implemented")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was renamed to build_sqlalchemy_uri:

sqlalchemy_uri = engine_spec.build_sqlalchemy_uri(

Suggested change
def build_sqlalchemy_url(cls, parameters: Any) -> str:
raise NotImplementedError("build_sqlalchemy_url is not implemented")
def build_sqlalchemy_uri(cls, parameters: Any) -> str:
raise NotImplementedError("build_sqlalchemy_uri is not implemented")

Comment thread superset/db_engine_specs/base.py Outdated
Comment thread superset/db_engine_specs/base.py Outdated
Comment thread superset/db_engine_specs/base.py
Comment thread superset/models/core.py Outdated
hughhhh and others added 7 commits May 18, 2021 14:07
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
@pull-request-size pull-request-size Bot added size/S and removed size/M labels May 18, 2021
@hughhhh hughhhh mentioned this pull request May 18, 2021
8 tasks
Comment thread superset/models/core.py

@property
def parameters(self) -> Dict[str, Any]:
# Build parameters if db_engine_spec is a subclass of BasicParametersMixin
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
# Build parameters if db_engine_spec is a subclass of BasicParametersMixin
# Build parameters if db_engine_spec supports it

@hughhhh hughhhh merged commit ec0a792 into master May 19, 2021
@github-actions
Copy link
Copy Markdown
Contributor

Ephemeral environment shutdown and build artifacts deleted.

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@rusackas rusackas deleted the add-parameters-api branch January 30, 2023 19:21
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 First shipped in 1.3.0 labels Mar 12, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
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 🚢 1.3.0 First shipped in 1.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants