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.
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
Support additional connection methods #323
base: main
Are you sure you want to change the base?
Support additional connection methods #323
Changes from 31 commits
ab4b269
ff9fdfd
fbd5731
4f98546
f724708
5319b90
16666db
30ae0b5
de7c411
bfe8678
c8a18d8
40e0fe5
c74d3ee
66c1594
54bc39f
3ed9876
4bb97ab
cfad7ff
b1c8e00
12eb89b
9a319ac
d3113ca
ccfebc8
1dc3ebe
b43d20c
93947e1
fb60268
c710803
17fcb03
daaf05b
0947784
b1b09ce
971c957
674d754
5856fe2
2047bbf
4622210
857294a
c901bb7
0d89af6
b66b58d
6eb89e4
de7e0e7
0b60812
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
if these are okta specific can we add a prefix like
okta_
* to indicate that?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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Same comment as above about testing for IDP
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"