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
Use profile specified in --profile with dbt init #7450
Use profile specified in --profile with dbt init #7450
Changes from 3 commits
9179070
2755820
f8d66f8
7e921ce
9a58a15
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.
User experience
From a user experience perspective, I don't think we need to raise an error if the specified profile is not found. I'd rather just create it anytime it doesn't exist.
Implementation
I defer to whoever ends up being the code reviewer for this, but see below for some suggestions for refactoring.
The
run
method is long and has a lot of conditionals, which makes it harder to read. Refactoring this would make it easier to maintain in the future.So I'd suggest refactoring this logic into its own method (similar to how
check_if_can_write_profile
is its own method). Maybe something similar this (completely untested!):Then this method can be applied in one or more places to use the specified profile if it exists (and create it otherwise).
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.
For user experience, I think more input is needed. The risk with creating the profile if it does not exist is the classic typo problem, where a misspelling creates a new profile instead of using the one the user actually wanted. The requirements said "existing profile" so I put in the check.
For implementation, the test is only done once currently, but shrinking a big method is usually a good idea. Refactored.
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.
Did you push the refactored code @ezraerb?
This is the most recent that I'm seeing:
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.
Still working on a few other comments, so have not pushed yet.
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.
Changes have been pushed.
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.
My first instinct was to agree with @dbeatty10 on this point:
I definitely appreciate the typo annoyance:
dbt init --profile defaut
would lead to the writing of a whole newdefaut
profile, when you intended to use the existingdefault
profile. In that case, it would be better to get an explicit error.But it means that, when initializing a new project from scratch, you have exactly two options:
--profile
flag: Initialize a new project and a new profile, with the same name as your project.--profile
flag. It must match an existing profile.If we took Doug's recommendation, there would be three options:
--profile
flag to initialize a new project and a new profile. The profile name will match your supplied project name (reasonable default behavior).--profile
flag, and its value matches an existing profile: Initialize a new project, do not write a new profile.--profile
flag, and its value doesn't match an existing profile: Initialize a new project and a new profile. The profile name will match what you passed into the--profile
flag.We could even take this one step further, and provide the same flexibility that we offer when running
dbt init
within an existing project:I don't have strong feelings either way. As a heuristic to make this determination, I'm thinking about how there are two "modes" of
dbt init
:dbt init <project_name> --skip-profile-setup
, ordbt init <project_name> --profile <existing_profile_name>
.This
--profile
flag feels designed for persona / use case (2). The first user is less likely to want fine-grained control over exactly how the profile is being named — we should provide the easiest path from start to finish, with some sensible defaults along the way. And second user (slightly more experienced) is less likely to want the interactive walkthrough for setting up their profile.Which is to say: I'm convinced enough that the behavior implemented in this PR is an acceptable user experience. We'll need to document the behavior in a new "Existing profile" section here: https://docs.getdbt.com/reference/commands/init
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.
Curious why Black didn't reformat this or the line below it to be the same.
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.
Can we make this a fixture in a constant at file top or in a
fixture.py
?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.
Checking the other init tests shows that all of them use this pattern. Its probably worth getting more opinions before changing it.