Skip to content
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

[CT-1737] Support all "global" flags after all subcommands #6497

Closed
Tracked by #8600
jtcohen6 opened this issue Jan 2, 2023 · 6 comments · Fixed by #8670 · May be fixed by #10437
Closed
Tracked by #8600

[CT-1737] Support all "global" flags after all subcommands #6497

jtcohen6 opened this issue Jan 2, 2023 · 6 comments · Fixed by #8670 · May be fixed by #10437
Assignees
Labels
backport 1.5.latest backport 1.6.latest cli enhancement New feature or request Impact: CLI user docs [docs.getdbt.com] Needs better documentation

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jan 2, 2023

See comment below for updated proposal

We currently distinguish between two types of flags:

  • those which precede the subcommand ("global"), e.g. --no-use-color
  • those which follow the subcommand, and may have different behavior depending on the subcommand, e.g. --select, --full-refresh

IMO this is pretty confusing for end users! We should really seek to support flags in any order (and resolve any resulting ambiguities by actually renaming flags). I know we implemented the new CLI modeling in accordance with the old CLI model — parity is a good goal to shoot for with any refactor — but it would be great to just support both of these, seamlessly and equivalently:

$ dbt --no-use-color run --select one_model
$ dbt run --select one_model --no-use-color
@jtcohen6 jtcohen6 added enhancement New feature or request cli Team:Execution labels Jan 2, 2023
@github-actions github-actions bot changed the title Support arbitrary order for CLI flags [CT-1737] Support arbitrary order for CLI flags Jan 2, 2023
@iknox-fa
Copy link
Contributor

Per BLG on 1/17/23 converting to a spike.. this is a lot of work and has a lot of potential pitfalls.

@iknox-fa iknox-fa changed the title [CT-1737] Support arbitrary order for CLI flags [CT-1737] [Spike] Support arbitrary order for CLI flags Jan 17, 2023
@jtcohen6 jtcohen6 added the spike label Jan 18, 2023
@github-actions
Copy link
Contributor

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please comment on the issue or else it will be closed in 7 days.

@github-actions github-actions bot added the stale Issues that have gone stale label Jul 18, 2023
@github-actions github-actions bot removed the stale Issues that have gone stale label Jul 20, 2023
@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Aug 4, 2023

One way to accomplish this goal: all global/root parameters should also be defined on every single subcommand, so that you can pass them before or after that subcommand.

Considerations:

  • We already have some logic that raises an error if you try to pass it in both places: dbt --fail-fast run --no-fail-fast
  • We would want to ensure complete coverage going forward: do this programmatically (”global” decorator-of-decorators?), or have a test to ensure that every relevant param is shared across both sets

Without being too clever, I think this could be a reasonable way to go for now.

Then, we should update our documentation to encourage that users always pass flags after the subcommand.

@jtcohen6 jtcohen6 added the user docs [docs.getdbt.com] Needs better documentation label Aug 10, 2023
@graciegoheen
Copy link
Contributor

graciegoheen commented Aug 14, 2023

From refinement meeting:

  • would we want to deprecate the global parameters in the future? maybe! when we do, we'll add deprecation warnings before doing so
  • will need a documentation update
  • update old tests, write new tests
  • for click, you have to inject the flag in, not as easy to just add extra flags in

@jtcohen6 jtcohen6 changed the title [CT-1737] [Spike] Support arbitrary order for CLI flags [CT-1737] Support arbitrary order for CLI flags Sep 7, 2023
@jtcohen6 jtcohen6 changed the title [CT-1737] Support arbitrary order for CLI flags [CT-1737] Support all "global" flags after all subcommands Sep 7, 2023
@jtcohen6 jtcohen6 removed the spike label Sep 7, 2023
@aranke
Copy link
Member

aranke commented Sep 18, 2023

@jtcohen6 What's the expected behavior in the following scenarios:

  1. A flag is present at global and sub-command levels, but has the same value at each level?
  2. A flag is present at global and sub-command levels, but has different values at each level?

@jtcohen6
Copy link
Contributor Author

@aranke Current state (since v1.5) is that we raise an error in both cases. I think we should preserve that behavior.

# Ensure that any params sourced from the commandline are not present more than once.
# Click handles this exclusivity, but only at a per-subcommand level.
seen_params = []
for param in _get_params_by_source(ctx, ParameterSource.COMMANDLINE):
if param in seen_params:
raise DbtUsageException(
f"{param.lower()} was provided both before and after the subcommand, it can only be set either before or after.",
)
seen_params.append(param)

(Implemented in #7303)

github-actions bot pushed a commit that referenced this issue Sep 28, 2023
aranke added a commit that referenced this issue Oct 11, 2023
aranke added a commit that referenced this issue Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.5.latest backport 1.6.latest cli enhancement New feature or request Impact: CLI user docs [docs.getdbt.com] Needs better documentation
Projects
None yet
4 participants