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 --adapter to dbt init, to create sample profiles.yml bas… #2594

Merged
merged 14 commits into from
Aug 11, 2020
Merged

Conversation

brunomurino
Copy link
Contributor

@brunomurino brunomurino commented Jun 25, 2020

resolves #2533

Description

Added option '--adapter' to dbt init, to create sample profiles.yml based on chosen adapter.

It gets the sample profiles.yml from the adapter 'includes' folder

Related PRs:

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 cla-bot bot added the cla:yes label Jun 25, 2020
@brunomurino
Copy link
Contributor Author

I think the test is failing because the container "fishtownanalytics/test-container:7" doesn't have the samples_profiles.yml on the postgres folder. How can I change that?

@beckjake
Copy link
Contributor

beckjake commented Jun 26, 2020

I think the test is failing because the container "fishtownanalytics/test-container:7" doesn't have the samples_profiles.yml on the postgres folder. How can I change that?

I might be wrong about this, but I think the issue is that the setup.py file for each plugin needs to add the path to the sample_profiles.yml to package_data. This gets me basically every time I add a static file to dbt.

@brunomurino
Copy link
Contributor Author

@beckjake Thanks for the heads up! It all worked now. Ready to be reviewed, I think!

@drewbanin drewbanin requested a review from jtcohen6 July 1, 2020 13:26
@beckjake
Copy link
Contributor

beckjake commented Jul 1, 2020

@brunomurino I (finally) got the branch this depends upon merged into dev/marian-anderson. Could you change the target branch for this PR to dev/marian-anderson please?

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

@brunomurino This is a great change! I have a few comments around keeping the sample profiles consistent between the different adapters.

  • The top-level profile name should be default for all of them, so that it matches the profile name specified in the starter project and just works from the get-go. I dislike that the starter project has profile: 'default' and name: 'my_new_project', but I think we would need to write a bunch more code to make that dynamic.
  • Let's include bracketed entries (password: [password]) instead of fake entries (password: pa55word)
  • Let's always include two outputs, dev and prod. I like how it demonstrates the options and extensibility available, especially for plugins that have multiple connection methods.
    • Postgres: dev and prod, with different usernames ([dev_username] and [prod_username]), passwords, and schemas
    • Redshift: dev (password-based), prod (iam)
    • Snowflake: dev (user-password), prod (keypair)
    • BigQuery: dev (oauth), prod (service_account) on BigQuery

I'm curious to hear what you think about the above. I'm flexible on the specifics, but definitely set on wanting consistency here and for the plugins as well (dbt-labs/dbt-spark#98 + dbt-labs/dbt-presto#26)

@brunomurino brunomurino changed the base branch from fix/adapter-macro-namespacing to dev/marian-anderson July 4, 2020 13:14
@brunomurino
Copy link
Contributor Author

@brunomurino This is a great change! I have a few comments around keeping the sample profiles consistent between the different adapters.

  • The top-level profile name should be default for all of them, so that it matches the profile name specified in the starter project and just works from the get-go. I dislike that the starter project has profile: 'default' and name: 'my_new_project', but I think we would need to write a bunch more code to make that dynamic.

  • Let's include bracketed entries (password: [password]) instead of fake entries (password: pa55word)

  • Let's always include two outputs, dev and prod. I like how it demonstrates the options and extensibility available, especially for plugins that have multiple connection methods.

    • Postgres: dev and prod, with different usernames ([dev_username] and [prod_username]), passwords, and schemas
    • Redshift: dev (password-based), prod (iam)
    • Snowflake: dev (user-password), prod (keypair)
    • BigQuery: dev (oauth), prod (service_account) on BigQuery

I'm curious to hear what you think about the above. I'm flexible on the specifics, but definitely set on wanting consistency here and for the plugins as well (fishtown-analytics/dbt-spark#98 + fishtown-analytics/dbt-presto#26)

I agree and have updated the sample profiles!

@brunomurino
Copy link
Contributor Author

@jtcohen6 Updated the sample profiles on spark and presto plugins as well, let me me know what do you think.

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Hey @brunomurino, sorry for the delay! Thanks for humoring me on making the profiles more consistent + thorough. I left two quick comments on the redshift + snowflake profiles. Then we can merge this (+ spark + presto) for 0.18.0 :)

@brunomurino
Copy link
Contributor Author

@jtcohen6 No worries at all and sorry for the delay. Feel free to give any more suggestions!

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Thanks @brunomurino! Very last ones, I promise.

plugins/postgres/dbt/include/postgres/sample_profiles.yml Outdated Show resolved Hide resolved
plugins/postgres/dbt/include/postgres/sample_profiles.yml Outdated Show resolved Hide resolved
plugins/redshift/dbt/include/redshift/sample_profiles.yml Outdated Show resolved Hide resolved
plugins/redshift/dbt/include/redshift/sample_profiles.yml Outdated Show resolved Hide resolved
@brunomurino
Copy link
Contributor Author

@jtcohen6 Happy to help! I also checked the presto-spark repos and they're already in line with your suggestions on this repo!

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Looks good! I found the tiniest last change, I'm just going to add as suggestions. Approve at your convenience and I'll merge the PR. Thanks for your patience and for your contribution!

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Approved once suggestions are committed

plugins/postgres/dbt/include/postgres/sample_profiles.yml Outdated Show resolved Hide resolved
plugins/postgres/dbt/include/postgres/sample_profiles.yml Outdated Show resolved Hide resolved
plugins/redshift/dbt/include/redshift/sample_profiles.yml Outdated Show resolved Hide resolved
plugins/redshift/dbt/include/redshift/sample_profiles.yml Outdated Show resolved Hide resolved
@jtcohen6 jtcohen6 merged commit 89775fa into dbt-labs:dev/marian-anderson Aug 11, 2020
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.

Write database-specific sample profiles.yml file in dbt init
3 participants