[CT-2069] Unexpected behavior with global/subcommand flags

[jtcohen6]

For backwards compatibility with older behavior, some flags are implemented at both the "global" ("parent") level, and also at the "subcommand" ("child") level. We have some tricky logic to handle this potential duplication:

dbt-core/core/dbt/cli/flags.py

Lines 70 to 95 in 7774857

	def assign_params(ctx, params_assigned_from_default):
	"""Recursively adds all click params to flag object"""
	for param_name, param_value in ctx.params.items():
	# TODO: this is to avoid duplicate params being defined in two places (version_check in run and cli)
	# However this is a bit of a hack and we should find a better way to do this
	# N.B. You have to use the base MRO method (object.__setattr__) to set attributes
	# when using frozen dataclasses.
	# https://docs.python.org/3/library/dataclasses.html#frozen-instances
	if hasattr(self, param_name.upper()):
	if param_name not in EXPECTED_DUPLICATE_PARAMS:
	raise Exception(
	f"Duplicate flag names found in click command: {param_name}"
	)
	else:
	# Expected duplicate param from multi-level click command (ex: dbt --full_refresh run --full_refresh)
	# Overwrite user-configured param with value from parent context
	if ctx.get_parameter_source(param_name) != ParameterSource.DEFAULT:
	object.__setattr__(self, param_name.upper(), param_value)
	else:
	object.__setattr__(self, param_name.upper(), param_value)
	if ctx.get_parameter_source(param_name) == ParameterSource.DEFAULT:
	params_assigned_from_default.add(param_name)
	if ctx.parent:
	assign_params(ctx.parent, params_assigned_from_default)

On feature/click-cli branch, I'm observing behavior where:

• parent flag alone isn't respected
• child flag alone is respected
• flag provided to both parent + child, parent flag takes precedence and is respected

Expected behavior:

• parent flag alone is (!) respected
• child flag alone is respected
• if both parent + child flags are passed, raise an error (ideal) — but if needed, we can document that the parent flag always wins

Example

Create a new dbt project, and add require-dbt-version: "1.4.1" to dbt_project.yml.

$ python3 -m dbt.cli.main --no-version-check parse
...
dbt.exceptions.DbtProjectError: Runtime Error
  This version of dbt is not supported with the 'test' package.
    Installed version of dbt: =1.5.0-a1
    Required version of dbt for 'test': ['=1.4.1']
  Check for a different version of the 'test' package, or run dbt again with --no-version-check

$ python3 -m dbt.cli.main parse --no-version-check
23:52:24  Performance info: target/perf_info.json

$ python3 -m dbt.cli.main --no-version-check parse --version-check
23:52:27  Performance info: target/perf_info.json

[stu-k]

@jtcohen6 Can we choose only the option where there are not parent flags? LIke dbt run --flag and never dbt --flag run?

[jtcohen6]

@stu-k That would be a significant breaking change for a large number of existing CLI users, so I don't see it as a viable option.

[stu-k]

@jtcohen6 Would it be fair to ask that a flag be defined in either the parent or child level, not both?

✅ dbt --full-refresh run OR (not both) dbt run --full-refresh
🚫 dbt --full-refresh run --full-refresh

[jtcohen6]

It's clear that this is the intent of the assign_params method, but it's not currently working.

Acceptance criteria for this ticket:

• P0: Respect "parent"-level flag when passed alone
• P1: Raise an exception if the same flag is passed in both places. Only do this if it's a minor lift. Otherwise we document the current behavior.

[jtcohen6]

@ChenyuLInx says:

dbt-core/core/dbt/cli/flags.py

Line 87 in 7774857

if ctx.get_parameter_source(param_name) != ParameterSource.DEFAULT:

This is the wrong ctx. It is currently the context of the child, and it should be the context of the parent.

[iknox-fa]

@jtcohen6 Would it be fair to ask that a flag be defined in either the parent or child level, not both?

✅ dbt --full-refresh run OR (not both) dbt run --full-refresh 🚫 dbt --full-refresh run --full-refresh

@jtcohen6 Just to be clear-- your 👍 on this comment means there is no legitimate use case where the same flag will be populated on multiple levels? I ask because right now we have logic here specifically to support that happening.

[jtcohen6]

@jtcohen6 Just to be clear-- your 👍 on this comment means there is no legitimate use case where the same flag will be populated on multiple levels? I ask because right now we have logic here specifically to support that happening.

IMO, if the same flag is passed at both the "parent" and the "child" levels:

• Ideal: Raise an error. Message something like: --version-check was provided both before and after the subcommand, it can only be set either before or after.
• Acceptable: We have a consistent behavior whereby either the "parent" or the "child" always wins, and we document that behavior. I have a slight preference for the "parent" winning (status quo), since that's where we document/suggest putting these
  global" flags currently. I could also see the argument for the "child" flag winning since it's passed later (see rationale below).

I understand that standard CLI behavior allows you to pass the same flag multiple times, and the last one wins, e.g. dbt --debug --no-debug test. In the fullness of time, if our CLI could remove the distinction between parent/child flags (#6497), then picking the last instance of the flag to win seems reasonable. For now, I'd rather opt for behavior that's transparent & explicit for the end user.