refactor: pass all properties to validate_parameters #21487
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eschutho
force-pushed
the
elizabeth/database_properties
branch
from
c9a3d80 to
b488f5b
Compare
eschutho
force-pushed
the
elizabeth/database_properties
branch
from
b488f5b to
0d7b038
Compare
Codecov Report
@@ Coverage Diff @@
## master #21487 +/- ##
==========================================
- Coverage 66.79% 65.15% -1.65%
==========================================
Files 1799 1791 -8
Lines 68829 68587 -242
Branches 7314 7320 +6
==========================================
- Hits 45976 44689 -1287
- Misses 20974 22008 +1034
- Partials 1879 1890 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
eschutho
force-pushed
the
elizabeth/database_properties
branch
from
0d7b038 to
a4726e2
Compare
|
/testenv up |
|
@eschutho Ephemeral environment spinning up at http://34.219.255.3:8080. Credentials are |
|
test the db connection form in ephemeral env, LGTM |
jinghua-qa
approved these changes
Sep 28, 2022
eschutho
force-pushed
the
elizabeth/database_properties
branch
from
87cd1ce to
986471e
Compare
9 tasks
eschutho
force-pushed
the
elizabeth/database_properties
branch
from
986471e to
aaeb7a5
Compare
|
Ephemeral environment shutdown and build artifacts deleted. |
hughhhh
pushed a commit
to preset-io/superset
that referenced
this pull request
Oct 7, 2022
(cherry picked from commit e98943e)
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some databases require values that aren't in the sqlalchemy uri in order to connect. I'm passing in all properties from the validate_params api into the db_engine_spec for validation, instead of just the
parametersso that we can continue to keep the client side api structure as similar as possible to the db database model and not have to add the same values in both the root db payload and parameters in order to do validation. This is some pre-work for adding in the databricks form that requires http_path from extras for connection.Hopefully this change will help clarify the difference between parameters, and when something is a parameter in the db model and when it's not.
I'm also adding the driver to the db model, because as per @betodealmeida's change in #21171 if an engine can have multiple drivers, like databricks, we're going to need to pass that value in from the client side when creating/editing a db in order for us to find the correct db_engine_spec.
TESTING INSTRUCTIONS
Database creation with the new forms should work as expected.
ADDITIONAL INFORMATION