Support additional connection methods #323
Conversation
… define a _get_connect_method method to leverage Redshift python connector to retrieve the connect method
…h due to in-built support in Redshift Python Connector
…d modifying multi statement execution
dbt/adapters/redshift/connections.py
Outdated
| } | ||
| if method == RedshiftConnectionMethod.IAM or method == RedshiftConnectionMethod.DATABASE: | ||
| kwargs["host"] = self.credentials.host | ||
| kwargs["region"] = self.credentials.host.split(".")[2] |
There was a problem hiding this comment.
if the user passes a region that's different from the region in the host should we raise an exception?
There was a problem hiding this comment.
Good catch, we could also default to using region from host, in the scenario that host is provided. So if provided region and region in host are different we use the region from host. But we could also throw an error. What are your thoughts?
| ) | ||
| if self.credentials.role: | ||
| c.cursor().execute("set role {}".format(self.credentials.role)) | ||
| return c | ||
|
|
||
| return connect | ||
| elif method == RedshiftConnectionMethod.IDP: |
There was a problem hiding this comment.
have we done any local testing of these new connection methods? Ideally we would be able to cover these with integration tests but that may be unrealistic
There was a problem hiding this comment.
Yes, I've done local testing for these new connection methods. What I've done for local testing - Got some credentials from someone who manages redshift_connector and edited the profiles.yml to connect to Redshift.
There was a problem hiding this comment.
Not sure how feasible integration tests are - since we got these credentials from another team and we don't own these credentials. Ideally we should spend some time to create these credentials ourselves so that the integration tests don't depend on another team - but that may be out of scope for right now.
dbt/adapters/redshift/connections.py
Outdated
| return c | ||
|
|
||
| return connect | ||
| elif self.credentials.credentials_provider == "OktaCredentialsProvider": |
There was a problem hiding this comment.
Same comment as above about testing for IDP
| region=self.credentials.region, | ||
| database=self.credentials.database, | ||
| cluster_identifier=self.credentials.cluster_id, | ||
| credentials_provider="OktaCredentialsProvider", |
There was a problem hiding this comment.
nit: does this need to be hardcoded? Looks like we already check that self.credentials.credentials_provider == "OktaCredentialsProvider" above
There was a problem hiding this comment.
Good call. Might also be a good idea to allow more wiggle room for the customer to use these credentials. I think we can hardcode credentials_provider="OktaCredentialsProvider" in the connection call, and allow for multiple variations of Okta from the customer(both "okta" and "oktacredentialprovider" should be accepted with case insensitivity), instead of requiring customers to input excatly "OktaCredentialsProvider"
|
Hello guys, any chance to push it forward? We really miss the support for the generic SAML Browser IdP plugin, but the MR seems to be stale for months 😕 |
|
Any update here? Especially the generic IdP is very much needed in our case - would be great to see this moving forward. |
|
This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days. |
|
Bump |
|
This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days. |
|
Although we are closing this PR as stale, it can still be reopened to continue development. Just add a comment to notify the maintainers. |
resolves #332
Description
Checklist
changie newto create a changelog entry