Set Flags from UserConfig #6266
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
MichelleArk
force-pushed
the
arky/click-flags-user-config
branch
from
eb300c2
to
25cb6a0
Compare
MichelleArk
changed the title
MichelleArk
force-pushed
the
arky/click-flags-user-config
branch
from
25cb6a0
to
302b81a
Compare
core/dbt/cli/flags.py
Outdated
|
|
||
| # Hard coded flags | ||
| object.__setattr__(self, "WHICH", ctx.info_name) | ||
| object.__setattr__( | ||
| self, "WHICH", ctx.protected_args[0] if ctx.protected_args else ctx.info_name |
There was a problem hiding this comment.
I'm not sure I follow what this is doing... According to the docs protected_args is used for certain extra parsing scenarios and that doesn't sound like that we're looking for when using flags.WHICH?
(as far as I can tell WHICH is only used to identify the command invoked in tracking events)
There was a problem hiding this comment.
Here I was looking to get an accurate value for WHICH regardless of whether the context was based on the CLI group or a subcommand. I assume we'll want callers going through the CLI group context because it does a lot of setup (and even implements some CLI functionality, such as --version).
In either case the caller would be responsible for figuring out what to set name to when constructing a Context object programmatically, which is then used to set flags.VERSION:
>>> from dbt.cli.main import cli
>>> from dbt.cli.flags import Flags
# Caller would need to infer that "run" is the right name to set based on args.
>>> ctx = cli.make_context(cli.name, ["--partial-parse", "run"])
>>> flags = Flags(ctx)
>>> flags.WHICH
'cli'Perhaps invoked_subcommand is what we should use here, if available. However invoked_subcommand is only available if the command is actually being invoked, as opposed to based on the provided args. So it would be None in the example above.
I went with protected_args because of the docs hinting that it is used to implement nested parsing, but it does feel fragile as well.
There was a problem hiding this comment.
Related note - Currently on feature/click-cli, flags.WHICH is 'dbt' at this point, not the subcommand name when called directly. So it's technically not accurate unless we recreate flags further along in the subcommand method, which I understand we're looking to move away from doing. Using invoked_subcommand when available should at least fix the issue from the CLI interface.
There was a problem hiding this comment.
As far as I'm aware, the real function of flags.WHICH is to support conditional checks in user code for {{ if flags.WHICH = ... }}. That is, conditional behavior that only wants to run during certain commands, and not others.
links:
- the
flagsmodule is exposed to the context here - https://docs.getdbt.com/reference/dbt-jinja-functions/flags
- functional test showing this behavior
There was a problem hiding this comment.
Oh I'm glad we had this conversation! Yes invoked_subcommand is exactly what we should be using-- In my head that's what info_name did-- I probably C/P'd the wrong line in the original code (info_name is right above invoked_subcommand in the output of pp vars(flags)).
There was a problem hiding this comment.
Updated flags.WHICH to use invoked_subcommand if available, falling back on ctx.info_name. I think it'd still be worth looking into protected_args more but I'll punt that to a follow-up if that's alright; feels a bit out of scope for this work.
| object.__setattr__(self, "MP_CONTEXT", get_context("spawn")) | ||
|
|
||
| # Support console DO NOT TRACK initiave | ||
| object.__setattr__( | ||
| self, | ||
| "ANONYMOUS_USAGE_STATS", | ||
| False | ||
| if os.getenv("DO_NOT_TRACK", "").lower() in (1, "t", "true", "y", "yes") | ||
| if os.getenv("DO_NOT_TRACK", "").lower() in ("1", "t", "true", "y", "yes") |
There was a problem hiding this comment.
Good catch! env vars != ints. Ever.
flags with user config, flags.WHICH from invoked_subcommand if available
resolves ##6327
Description
UserConfig(if available) toFlags, preferringUserConfigvalues to environment variables and defaults.cli.FlagsAnother option considered was to implement resolvers for each user-config param in cli/resolvers.py and use them to set defaults in params.py. However, this approach would still need access to profiles_dir prior to Flags initialization, and be much less succinct.
Checklist
changie newto create a changelog entry