Skip to content

Use profile specified in --profile with dbt init #7450

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.

Sign up for GitHub

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

Merged
emmyoop merged 5 commits into from
Sep 15, 2023
Merged

Use profile specified in --profile with dbt init #7450

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9179070
Use profile specified in --profile with dbt init
ezraerb Apr 24, 2023
2755820
Update .changes/unreleased/Fixes-20230424-161642.yaml
ezraerb May 23, 2023
f8d66f8
merge origin/main
ezraerb May 25, 2023
7e921ce
Refactor run() method into functions, replace exit() calls with excep…
ezraerb Jun 7, 2023
9a58a15
Update help text for profile option
ezraerb Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
merge origin/main
  • Loading branch information
@ezraerb
ezraerb committed May 25, 2023
commit f8d66f87009559f5c7b5c468c24ded2cb3cafb2b
77 changes: 22 additions & 55 deletions core/dbt/task/init.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from pathlib import Path
import re
import shutil
import sys
from typing import Optional

import yaml
Expand Down Expand Up @@ -248,11 +249,11 @@ def get_valid_project_name(self) -> str:

return name

def create_new_project(self, project_name: str):
def create_new_project(self, project_name: str, profile_name: str):
self.copy_starter_repo(project_name)
os.chdir(project_name)
with open("dbt_project.yml", "r") as f:
content = f"{f.read()}".format(project_name=project_name, profile_name=project_name)
content = f"{f.read()}".format(project_name=project_name, profile_name=profile_name)
with open("dbt_project.yml", "w") as f:
f.write(content)
fire_event(
Expand Down Expand Up @@ -286,6 +287,7 @@ def run(self):
# When dbt init is run inside an existing project,
# just setup the user's profile.
profile_name = self.get_profile_name_from_current_project()
profile_specified = False
else:
# When dbt init is run outside of an existing project,
# create a new project and set up the user's profile.
Expand All @@ -294,11 +296,25 @@ def run(self):
if project_path.exists():
fire_event(ProjectNameAlreadyExists(name=project_name))
return
self.create_new_project(project_name)
profile_name = project_name

# Ask for adapter only if skip_profile_setup flag is not provided.
if not self.args.skip_profile_setup:
# If the user specified an existing profile to use, use it instead of generating a new one
user_profile_name = getattr(get_flags(), "PROFILE", None)
if user_profile_name:
# Verify it exists. Can't use the regular profile validation routine because it assumes
# the project file exists
raw_profiles = read_profile(profiles_dir)
if user_profile_name not in raw_profiles:
print("Could not find profile named '{}'".format(user_profile_name))
sys.exit(1)
profile_name = user_profile_name
profile_specified = True
else:
profile_name = project_name
profile_specified = False
self.create_new_project(project_name, profile_name)

# Ask for adapter only if skip_profile_setup flag is not provided and no profile to use was specified.
if not self.args.skip_profile_setup and not profile_specified:
Copy link
Contributor

@dbeatty10 dbeatty10 Jun 6, 2023

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!):

def check_if_profile_exists(self, profile_name: Optional[str] = None) -> bool:
    profile_exists = False  # assume it doesn't exist unless proven otherwise
    user_profile_name = getattr(get_flags(), "PROFILE", None)
    profiles_dir = get_flags().PROFILES_DIR
    if user_profile_name:
        raw_profiles = read_profile(profiles_dir)
        profile_exists = user_profile_name in raw_profiles
    return profile_exists

Then this method can be applied in one or more places to use the specified profile if it exists (and create it otherwise).

Copy link
Contributor Author

@ezraerb ezraerb Jun 6, 2023

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.

Copy link
Contributor

@dbeatty10 dbeatty10 Jun 7, 2023

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:
image

Copy link
Contributor Author

@ezraerb ezraerb Jun 7, 2023

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.

Copy link
Contributor Author

@ezraerb ezraerb Jun 7, 2023

Choose a reason for hiding this comment

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

Changes have been pushed.

Copy link
Contributor

@jtcohen6 jtcohen6 Sep 8, 2023

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:

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.

I definitely appreciate the typo annoyance: dbt init --profile defaut would lead to the writing of a whole new defaut profile, when you intended to use the existing default 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:

  1. Do not pass --profile flag: Initialize a new project and a new profile, with the same name as your project.
  2. Pass the --profile flag. It must match an existing profile.

If we took Doug's recommendation, there would be three options:

  1. Do not pass --profile flag to initialize a new project and a new profile. The profile name will match your supplied project name (reasonable default behavior).
  2. Pass the --profile flag, and its value matches an existing profile: Initialize a new project, do not write a new profile.
  3. Pass the --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:

The profile <profile-name> already exists in /Users/jerco/.dbt/profiles.yml. Do you wish to overwrite it? [y/N]: N

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:

  1. Interactive. Likely first time using dbt. Need to set up everything.
  2. Programmatic. Someone who has used dbt before, likely on this machine. Wants to skip the click interactivity and jump straight to dbt init <project_name> --skip-profile-setup, or dbt 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

fire_event(SettingUpProfile())
if not self.check_if_can_write_profile(profile_name=profile_name):
return
Expand All @@ -315,52 +331,3 @@ def run(self):
fire_event(InvalidProfileTemplateYAML())
adapter = self.ask_for_adapter_choice()
self.create_profile_from_target(adapter, profile_name=profile_name)
return

# When dbt init is run outside of an existing project,
# create a new project and set up the user's profile.
available_adapters = list(_get_adapter_plugin_names())
if not len(available_adapters):
print("No adapters available. Go to https://docs.getdbt.com/docs/available-adapters")
sys.exit(1)
project_name = self.get_valid_project_name()
project_path = Path(project_name)
if project_path.exists():
fire_event(ProjectNameAlreadyExists(name=project_name))
return

# If the user specified an existing profile to use, use it instead of generating a new one
user_profile_name = getattr(get_flags(), "PROFILE", None)
if user_profile_name:
# Verify it exists. Can't use the regular profile validation routine because it assumes
# the project file exists
raw_profiles = read_profile(profiles_dir)
if user_profile_name not in raw_profiles:
print("Could not find profile named '{}'".format(user_profile_name))
sys.exit(1)

self.copy_starter_repo(project_name)
os.chdir(project_name)
with open("dbt_project.yml", "r+") as f:
content = f"{f.read()}".format(
project_name=project_name,
profile_name=user_profile_name if user_profile_name else project_name,
)
f.seek(0)
f.write(content)
f.truncate()

# If an existing profile to use was not provided, generate a profile
# Ask for adapter only if skip_profile_setup flag is not provided.
if not user_profile_name and not self.args.skip_profile_setup:
if not self.check_if_can_write_profile(profile_name=project_name):
return
adapter = self.ask_for_adapter_choice()
self.create_profile_from_target(adapter, profile_name=project_name)
fire_event(
ProjectCreated(
project_name=project_name,
docs_url=DOCS_URL,
slack_url=SLACK_URL,
)
)
18 changes: 18 additions & 0 deletions tests/functional/init/test_init.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,24 @@ def test_init_provided_project_name_and_skip_profile_setup(
)


class TestInitInsideProjectAndSkipProfileSetup(TestInitInsideOfProjectBase):
@mock.patch("dbt.task.init._get_adapter_plugin_names")
@mock.patch("click.confirm")
@mock.patch("click.prompt")
def test_init_inside_project_and_skip_profile_setup(
self, mock_prompt, mock_confirm, mock_get, project, project_name
):
manager = mock.Mock()
manager.attach_mock(mock_prompt, "prompt")
manager.attach_mock(mock_confirm, "confirm")

assert Path("dbt_project.yml").exists()

# skip interactive profile setup
run_dbt(["init", "--skip-profile-setup"])
assert len(manager.mock_calls) == 0


class TestInitOutsideOfProjectWithSpecifiedProfile(TestInitOutsideOfProjectBase):
@mock.patch("dbt.task.init._get_adapter_plugin_names")
@mock.patch("click.prompt")
Expand Down
You are viewing a condensed version of this merge commit. You can view the full changes here.