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

Add run-operation to click CLI (#5552) #6656

Merged
merged 4 commits into from
Jan 20, 2023

Conversation

jtcohen6
Copy link
Contributor

resolves #5552

Sorry, just couldn't let others have all the fun...

L'engagement

  • Add RunOperationTask to click CLI
  • Add macro positional argument

Le tour

Borrowing 63870d5 locally, to avoid the error Object of type SpawnContext is not JSON serializable:

$ python3 -m pytest tests/functional/run_operations/
==================================== test session starts ====================================
platform darwin -- Python 3.10.9, pytest-7.2.0, pluggy-1.0.0
rootdir: /Users/jerco/dev/product/dbt-core, configfile: pytest.ini
plugins: csv-3.0.0, logbook-1.2.0, xdist-3.1.0, flaky-3.7.0, mock-3.10.0, dotenv-0.5.2, cov-4.0.0
collected 10 items

tests/functional/run_operations/test_run_operations.py ..........                     [100%]

==================================== 10 passed in 8.47s =====================================

🎩 Le prestige

{% macro say_hello(whomst="world", strong=False) %}
    {{ print("Hello " ~ whomst ~ (" !" if strong else "")) }}
{% endmacro %}
$ python3 -m dbt.cli.main run-operation say_hello
Hello world
$ python3 -m dbt.cli.main run-operation project_funway.say_hello
Hello world
$ python3 -m dbt.cli.main run-operation say_hello --args 'whomst: Jeremy'
Hello Jeremy
$ python3 -m dbt.cli.main run-operation say_hello --args '{"whomst": "Wisconsin", "strong": true}'
Hello Wisconsin !

Checklist

@jtcohen6 jtcohen6 requested a review from a team January 19, 2023 09:53
@jtcohen6 jtcohen6 requested a review from a team as a code owner January 19, 2023 09:53
@jtcohen6 jtcohen6 requested review from stu-k and ChenyuLInx January 19, 2023 09:53
@cla-bot cla-bot bot added the cla:yes label Jan 19, 2023
@jtcohen6 jtcohen6 linked an issue Jan 19, 2023 that may be closed by this pull request
@jtcohen6 jtcohen6 force-pushed the jerco/click-run-operation branch from 7f26774 to 4878009 Compare January 19, 2023 10:14
@@ -349,6 +350,7 @@ def run(ctx, **kwargs):

# dbt run operation
@cli.command("run-operation")
@click.argument("macro")
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit: could we move this down under @click.pass_context so that all the params are logically grouped together?

Copy link
Contributor

@MichelleArk MichelleArk Jan 20, 2023

Choose a reason for hiding this comment

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

also - we should probably define this in core/dbt/cli/params.py alongside the other params for consistency / reuse.

edit, upon a bit more thinking:
A downside to that is that it wont be apparent from the command definition which parameters are arguments vs options. Also, I could see an argument for defining commands and arguments together given arguments can't provide their own help text and are documented by the command.

Alls to say - I think defining the argument here is actually fine, especially given that macros is only used in one command at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with first nit! I'll make the order match the CLI order

For the second - two reasons I prefer keeping it alongside the command definitions:

  1. Click arguments can't have documentation / help text, click's recommendation is to document them by mentioning them explicitly in the command help text
  2. We have very very few positional arguments in dbt commands. (I think it's just run-operation MACRO and init PROJECT_NAME. I just remembered the latter, and will add in this PR so we don't forget.) These are truly one-offs. IMO adding them as (reusable) params risks the misperception that they're widely used, versus keeping them alongside the only commands for which they're relevant.

Copy link
Contributor

@MichelleArk MichelleArk left a comment

Choose a reason for hiding this comment

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

just a small nit otherwise LGTM! thank you for picking this up!

@jtcohen6 jtcohen6 force-pushed the jerco/click-run-operation branch from 2e7f400 to b197bb6 Compare January 20, 2023 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-928] dbt run-operation works in click
2 participants