-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
fix small issues with flags and logger to support dbt-rpc #6990
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ | |
from typing import Set, List | ||
|
||
from click import Context, get_current_context, BadOptionUsage | ||
from click.core import ParameterSource | ||
from click.core import ParameterSource, Command, Group | ||
|
||
from dbt.config.profile import read_user_config | ||
from dbt.contracts.project import UserConfig | ||
|
@@ -49,13 +49,33 @@ def convert_config(config_name, config_value): | |
# This function should take care of converting the values from config and original | ||
# set_from_args to the correct type | ||
ret = config_value | ||
if config_name.lower() == "warn_error_options": | ||
if config_name.lower() == "warn_error_options" and type(config_value) == dict: | ||
ret = WarnErrorOptions( | ||
include=config_value.get("include", []), exclude=config_value.get("exclude", []) | ||
) | ||
return ret | ||
|
||
|
||
def args_to_context(args: List[str]) -> Context: | ||
"""Convert a list of args to a click context with proper hierarchy for dbt commands""" | ||
from dbt.cli.main import cli | ||
|
||
cli_ctx = cli.make_context(cli.name, args) | ||
# args would get converted during make context | ||
if len(args) == 1 and "," in args[0]: | ||
args = args[0].split(",") | ||
sub_command_name, sub_command, args = cli.resolve_command(cli_ctx, args) | ||
|
||
# handle source and docs group | ||
if type(sub_command) == Group: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we only have one level of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Planning on revisit all of this and add more things to it as we rethink the flags object. |
||
sub_command_name, sub_command, args = sub_command.resolve_command(cli_ctx, args) | ||
|
||
assert type(sub_command) == Command | ||
sub_command_ctx = sub_command.make_context(sub_command_name, args) | ||
sub_command_ctx.parent = cli_ctx | ||
return sub_command_ctx | ||
|
||
|
||
@dataclass(frozen=True) | ||
class Flags: | ||
def __init__(self, ctx: Context = None, user_config: UserConfig = None) -> None: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,6 @@ | |
from dbt.events.functions import fire_event | ||
from dbt.events.types import NodeCompiling, NodeExecuting | ||
from dbt.exceptions import DbtRuntimeError | ||
from dbt import flags | ||
from dbt.task.sql import SqlCompileRunner | ||
from dataclasses import dataclass | ||
from dbt.cli.resolvers import default_profiles_dir | ||
|
@@ -89,14 +88,18 @@ def get_dbt_config(project_dir, args=None, single_threaded=False): | |
target=getattr(args, "target", None), | ||
) | ||
|
||
# set global flags from arguments | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The functions in this file are still by large untested, gonna revisit to replace this file and test the interfaces we have to replace it. |
||
set_from_args(runtime_args, None) | ||
profile, project = load_profile_project(project_dir, profile_name) | ||
assert type(project) is Project | ||
|
||
config = RuntimeConfig.from_parts(project, profile, runtime_args) | ||
|
||
# Set global flags from arguments | ||
flags.set_from_args(args, config) | ||
# the only thing this set_from_args does differently than | ||
# the one above is that it pass runtime config over, I don't think | ||
# we need that. but leaving this for now for future reference | ||
|
||
# flags.set_from_args(runtime_args, config) | ||
|
||
# This is idempotent, so we can call it repeatedly | ||
dbt.adapters.factory.register_adapter(config) | ||
|
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.
Why is this change necessary? Shouldn't warn_error_options always be a dictionary?
Docs: https://docs.getdbt.com/reference/global-configs#warnings-as-errors
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.
There are other places that passes in something that's not a dict. I can do another round of verify it is a dict and update input from other places.
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.
I remember why, it is because the default value for
warn_error_options
in UserConfig is None, so if you pass in a UserConfig object the original logic would failThere 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.
👍 gotcha.
config_value is not None
would be more precise here then