Skip to content
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

Merged
merged 7 commits into from
Jun 24, 2020
Merged

added option to specify profile when connecting to redshift via IAM #2581

merged 7 commits into from
Jun 24, 2020

Conversation

brunomurino
Copy link
Contributor

@brunomurino brunomurino commented Jun 22, 2020

resolves #2437

Description

Added option to specify profile when connecting to redshift via IAM

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot
Copy link

cla-bot bot commented Jun 22, 2020

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

@cla-bot cla-bot bot added the cla:yes label Jun 22, 2020
@ghost ghost mentioned this pull request Jun 22, 2020
@jtcohen6
Copy link
Contributor

@brunomurino The errors from flake8 have to do with pep8 style. I pulled this out of the CircleCI logs:

flake8 run-test: commands[0] | /bin/bash -c '$(which flake8) --select=E,W,F --ignore=W504,E741 core/dbt plugins/*/dbt'
plugins/redshift/dbt/adapters/redshift/connections.py:60:80: E501 line too long (85 > 79 characters)
plugins/redshift/dbt/adapters/redshift/connections.py:90:80: E501 line too long (81 > 79 characters)
plugins/redshift/dbt/adapters/redshift/connections.py:98:80: E501 line too long (90 > 79 characters)
ERROR: InvocationError for command /bin/bash -c '$(which flake8) --select=E,W,F --ignore=W504,E741 core/dbt plugins/*/dbt' (exited with code 1)

If you run make test-unit locally, it should include flake8 testing as well.

@brunomurino brunomurino marked this pull request as ready for review June 23, 2020 07:55
Copy link
Contributor

@drewbanin drewbanin left a 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'
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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/

Copy link
Contributor

@beckjake beckjake left a 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
Comment on lines 1 to 4
### Next
- Added option to specify profile when connecting to Redshift via IAM


Copy link
Contributor

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Contributor

@beckjake beckjake left a 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.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Jacob Beck <beckjake@users.noreply.github.com>
@brunomurino
Copy link
Contributor Author

I think the test failed for an unrelated issue. Can someone rerun the tests?

@brunomurino
Copy link
Contributor Author

Is it all good with this now?

@jtcohen6
Copy link
Contributor

This looks great, @brunomurino. Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add AWS profile option to profiles.yml
4 participants