-
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
added option to specify profile when connecting to redshift via IAM #2581
added option to specify profile when connecting to redshift via IAM #2581
Conversation
Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA. In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin. CLA has not been signed by users: @landbay-brunomurino |
@brunomurino The errors from flake8 have to do with pep8 style. I pulled this out of the CircleCI logs:
If you run |
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.
One quick comment here, but overall, this looks great!
f"with profile {iam_profile}") | ||
boto_session = boto3.Session( | ||
profile_name=iam_profile, | ||
region_name='eu-west-1' |
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.
I think we may not want to hard-code this region name here. Do you think this should be an option in the profile? Or even better, can we document that the region should be specified in the configured AWS IAM profile?
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.
Ops, my bad, I didn't mean to add that, just pushed the fix. Yea, I think everything should be specified in the profile.
Is there a repo for the user documentation? Couldn't find anything in the dbt repo
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.
The documentation is maintained in this repository: https://github.com/fishtown-analytics/docs.getdbt.com/
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.
I have some changelog-formatting feedback, but the code looks good to me!
CHANGELOG.md
Outdated
### Next | ||
- Added option to specify profile when connecting to Redshift via IAM | ||
|
||
|
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.
Instead of adding this ### Next
section at the top, can you add a ### Features
section under the ## dbt 0.18.0 (Release TBD)
line, and add your line there? Please also add a link to the issue/PR, like the other features listed!
Also, please add a new contributors section to the ## dbt 0.18.0 (Release TBD)
section with your username and a link here. Check out lower down in the file for the exact formatting!
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.
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.
Two tiny suggestions on the changelog formatting, this looks good to me.
Co-authored-by: Jacob Beck <beckjake@users.noreply.github.com>
I think the test failed for an unrelated issue. Can someone rerun the tests? |
Is it all good with this now? |
This looks great, @brunomurino. Thank you for the contribution! |
resolves #2437
Description
Added option to specify profile when connecting to redshift via IAM
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.