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

move favor state arg to click #6774

Merged
merged 8 commits into from
Jan 30, 2023
Merged

Conversation

ChenyuLInx
Copy link
Contributor

resolves #6585

Tests related to --favor-state passes. There are still two fail but not related to --favor-state. Planning on resolving those at the very end.

tests/functional/defer_state/test_defer_state.py ....FF..                [100%]

@ChenyuLInx ChenyuLInx requested a review from a team as a code owner January 28, 2023 07:13
@ChenyuLInx ChenyuLInx requested review from nssalian and aranke January 28, 2023 07:13
@cla-bot cla-bot bot added the cla:yes label Jan 28, 2023
@github-actions
Copy link
Contributor

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.

@jtcohen6
Copy link
Contributor

@ChenyuLInx I figured it out! The tests fail for me locally, and then pass when I switch -m to -s in the arguments to run_dbt (83850f5)! It seems like this might be a genuine bug where we're not passing through --models/-m appropriately. That's important for backward compatibility.

Desirable behavior:

  • For almost all commands, --models should be a pure alias for --select (and the two should be mutually exclusive)
  • EXCEPTION: dbt list historically accepted both --select and --models, with slightly different behavior, which we've preserved for back compat

As a separate matter, docs generate --defer is indeed a bit of an odd duck:

  • docs generate --no-compileis effectively incompatible with--defer, because docs generaterelies on thecompile` step to perform the manifest state comparison/merge
  • Selection criteria for docs generate only currently impacts which nodes are compiled, not which nodes are catalogued. (We might want to change that: [CT-1303] Respect node selection in catalog queries run by docs generate #6014)
  • Personally, I'd imagine wanting to catalog whichever version of the node actually exists—my dev version if it exists, the prod version if it doesn't—ignoring selection logic entirely (24872f1)

That's obviously out of scope for this PR

Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

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

LGTM, one naming nit.

@@ -75,6 +75,13 @@
help="If set, defer to the state variable for resolving unselected nodes.",
)

favor_defer_state = click.option(
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this should be defer_favor_state or simply favor_state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

core/dbt/cli/params.py Outdated Show resolved Hide resolved
@ChenyuLInx ChenyuLInx closed this Jan 30, 2023
@ChenyuLInx ChenyuLInx reopened this Jan 30, 2023
@ChenyuLInx ChenyuLInx removed the request for review from nssalian January 30, 2023 18:48
@ChenyuLInx ChenyuLInx added the Skip Changelog Skips GHA to check for changelog file label Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Skip Changelog Skips GHA to check for changelog file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants