-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[BUGFIX] Add connect args to execution engine schema #6663
[BUGFIX] Add connect args to execution engine schema #6663
Conversation
👷 Deploy request for niobium-lead-7998 pending review.Visit the deploys page to approve it
|
@@ -1004,6 +1004,9 @@ class Meta: | |||
boto3_options = fields.Dict( | |||
keys=fields.Str(), values=fields.Str(), required=False, allow_none=True | |||
) | |||
connect_args = fields.Dict( |
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.
Would we be able to add connect_args
to ExecutionEngineConfig
class above as well?
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.
Otherwise the PR looks great. thanks for catching this @itaise 🙇🏼
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.
@Shinnnyshinshin thank you! done :)
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.
🚀
This PR is to solve this issue.
There is an issue that when creating SQLAlchemy execution engine, if the user send "connect_args" in the execution engine config, it is not passed to the "create_engine" method.
This is because this attribute is dropped when creating the ExecutionEngineConfigSchema.
I checked it locally and this PR solve the issue
Changes proposed in this pull request: