-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[FEATURE] SnowflakeDatasource update #10005
Changes from all commits
947d74d
fe40816
d39a82a
9353f15
8abc721
3f31ec8
34732f5
20b583b
7b4e41e
7ff0989
54ea2a6
6a52bf9
886602e
719bfed
20e879e
a63eb8c
268d262
10e07d9
0da2c21
999d0d9
5a430cd
226fc91
e26410c
3628293
245e02f
e850096
cd07e4c
ad6cfee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,8 +51,9 @@ | |
}, | ||
{ | ||
"type": "string", | ||
"writeOnly": true, | ||
"format": "password" | ||
"minLength": 1, | ||
"maxLength": 65536, | ||
"format": "uri" | ||
Comment on lines
+54
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This appears to be a duplicate of the following item? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One represents the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
}, | ||
{ | ||
"type": "string", | ||
|
@@ -538,7 +539,7 @@ | |
}, | ||
"ConnectionDetails": { | ||
"title": "ConnectionDetails", | ||
"description": "Information needed to connect to a Snowflake database.\nAlternative to a connection string.", | ||
"description": "Information needed to connect to a Snowflake database.\nAlternative to a connection string.\n\nhttps://docs.snowflake.com/en/developer-guide/python-connector/sqlalchemy#additional-connection-parameters", | ||
"type": "object", | ||
"properties": { | ||
"account": { | ||
|
@@ -564,10 +565,12 @@ | |
}, | ||
"database": { | ||
"title": "Database", | ||
"description": "`database` that the Datasource is mapped to.", | ||
"type": "string" | ||
}, | ||
"schema": { | ||
"title": "Schema", | ||
"description": "`schema` that the Datasource is mapped to.", | ||
"type": "string" | ||
}, | ||
"warehouse": { | ||
|
@@ -587,7 +590,9 @@ | |
"required": [ | ||
"account", | ||
"user", | ||
"password" | ||
"password", | ||
"database", | ||
"schema" | ||
], | ||
"additionalProperties": false | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit confused by this. Under what circumstances would
self.template_str
not be set by the time we get here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this confused me too.
When the
ConfigUri
is created using parse_object_as() or as part of standard pydantic model validation.ConfigUri.__new__
is called (sometimes without the full URL/template_str). It then builds the full URL. But we need that fullurl
to be set as thetemplate_str
attribute.https://github.com/pydantic/pydantic/blob/8333bd59dd3424811f4e73cdd6e9414fd15bb6d7/pydantic/v1/networks.py#L183-L185
Config.__init__
can be called with 2 ways and we need to account for both.We do this by overriding
AnyUrl.__new__
+ConfigStr.__init__
template_str
template_str
, without URL part keywordsWithout this added logic the tests fail.