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

Add db_groups and autocreate flags to Redshift config #2262

Merged
merged 1 commit into from
Apr 1, 2020

Conversation

kyleabeauchamp
Copy link
Contributor

Resolves #1995

Description

This PR adds the db_groups and autocreate flags in redshift configurations.

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 Mar 28, 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: @kyleabeauchamp

@kyleabeauchamp
Copy link
Contributor Author

I believe I signed the CLA just prior to opening this PR, so perhaps it hasn't processed yet.

@beckjake
Copy link
Contributor

@cla-bot check

@cla-bot cla-bot bot added the cla:yes label Mar 30, 2020
@cla-bot
Copy link

cla-bot bot commented Mar 30, 2020

The cla-bot has been summoned, and re-checked this pull request!

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.

@kyleabeauchamp Thanks for your contribution! The cla-bot process has a manual step that we have to update - should be set now. I like this PR it looks great! Just a few things:

  • A couple changelog updates
  • Can you please add a unit test to make sure the parameters keep getting passed along properly in the future? There should be some relevant tests in test/unit/test_redshift_adapter.py.
  • It looks like flake8 is unhappy with some indentation:
plugins/redshift/dbt/adapters/redshift/connections.py:103:17: E123 closing bracket does not match indentation of opening bracket's line

CHANGELOG.md Outdated
@@ -5,6 +5,7 @@
- Added a `get-manifest` API call. ([#2168](https://github.com/fishtown-analytics/dbt/issues/2168), [#2232](https://github.com/fishtown-analytics/dbt/pull/2232))
- Support adapter-specific aliases (like `project` and `dataset` on BigQuery) in source definitions. ([#2133](https://github.com/fishtown-analytics/dbt/issues/2133), [#2244](https://github.com/fishtown-analytics/dbt/pull/2244))
- Users can now use jinja as arguments to tests. Test arguments are rendered in the native context and injected into the test execution context directly. ([#2149](https://github.com/fishtown-analytics/dbt/issues/2149), [#2220](https://github.com/fishtown-analytics/dbt/pull/2220))
- Added support for `db_groups` and `autocreate` flags in Redshift configurations. ([#1995](https://github.com/fishtown-analytics/dbt/issues/1995))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Added support for `db_groups` and `autocreate` flags in Redshift configurations. ([#1995](https://github.com/fishtown-analytics/dbt/issues/1995))
- Added support for `db_groups` and `autocreate` flags in Redshift configurations. ([#1995](https://github.com/fishtown-analytics/dbt/issues/1995)([#2262](https://github.com/fishtown-analytics/dbt/pull/2262))

Please add a link to this PR to the CHANGELOG

Also, please add an entry to Contributors with your username.

@kyleabeauchamp
Copy link
Contributor Author

I've fixed the changelog and lint, will take a look at tests this evening.

@kyleabeauchamp
Copy link
Contributor Author

OK I've added a very simple addition to one of the unit tests, let me know if there are any other good tests that can be modified. It seems like the unit tests mostly mock out the boto interfaces, so it's not clear if there's any simple way to test the full end-to-end.

@beckjake
Copy link
Contributor

beckjake commented Apr 1, 2020

@kyleabeauchamp Ok, I guess that's fair - we don't have any tests that mock out the boto get_cluster_credentials call and make sure that dbt passed values in. The existing test is probably sufficient.

The only way to really test this end-to-end is going to be to make an integration test out of it, which sounds like a lot.

One more thing - can you rebase and squash some of this against dev/octavius-catto? It seems like you've picked up merge commits going back as far as 0.12.1(!) and ideally this would just be one or two commits. Once you do that I'll kick off the full test suite and then we can merge this. Thanks!

@drewbanin
Copy link
Contributor

I can manually QA this if that would be helpful, and @beckjake we can totally set up integration test creds against our redshift instance if desired!

@beckjake
Copy link
Contributor

beckjake commented Apr 1, 2020

@drewbanin that would be awesome.

I think credentials to test this would be good too - should that be a seprate PR? I think a lot of it will be fiddly credentials/env var things in CI, no need for contributors to be stuck in the middle of that, right?

@drewbanin
Copy link
Contributor

Yeah - we can do that in a separate PR for sure. I'll ping you offline with creds in 1pass

@kyleabeauchamp
Copy link
Contributor Author

I did a git rebase (git rebase -i dev/octavius-catto) and now things look worse.

@kyleabeauchamp
Copy link
Contributor Author

kyleabeauchamp commented Apr 1, 2020

Ah, I rebased to the fork, not the upstream...

@kyleabeauchamp
Copy link
Contributor Author

OK I think I've rebased things appropriately.

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.

Looks great! I've rebased everything and kicked off tests, I'll get some integration tests open for this once it merges. Thanks for your contribution!

@beckjake beckjake merged commit aa0e58e into dbt-labs:dev/octavius-catto Apr 1, 2020
@drewbanin
Copy link
Contributor

nice one @kyleabeauchamp! :D

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.

Adding IAM parameters to redshift adapter
3 participants