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 ls to RPC server #3384

Merged
merged 10 commits into from
May 27, 2021
Merged

Add ls to RPC server #3384

merged 10 commits into from
May 27, 2021

Conversation

iknox-fa
Copy link
Contributor

@iknox-fa iknox-fa commented May 21, 2021

resolves #3311 and #3356

Description

Adds support for the list method in the RPC server
Adds new keys for unique_id and original_file_path to list task when invoked with json output

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label May 21, 2021
core/dbt/contracts/rpc.py Outdated Show resolved Hide resolved
@jtcohen6
Copy link
Contributor

This is working well for me in local testing!

A few comments:

  • As I noted above, could we switch the type of select to accept a string as well as a list? It should work exactly the same as models and exclude
  • For parity with the existing list task, I expected to see an error if models is set alongside select or resource_types, since these are mutually exclusive arguments. This shouldn't block merging the PR if it would be tricky to implement.
  • The p0 resolution to Add keys to list output, parametrize #3356 is all we need for now. Could we open a new issue to capture the p1 requirement, parametrizing the set of returned keys?

@iknox-fa
Copy link
Contributor Author

This is working well for me in local testing!

A few comments:

  • As I noted above, could we switch the type of select to accept a string as well as a list? It should work exactly the same as models and exclude
    Done!
  • For parity with the existing list task, I expected to see an error if models is set alongside select or resource_types, since these are mutually exclusive arguments. This shouldn't block merging the PR if it would be tricky to implement.
    Done!
  • The p0 resolution to Add keys to list output, parametrize #3356 is all we need for now. Could we open a new issue to capture the p1 requirement, parametrizing the set of returned keys?
    Done!

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

  • There's one last failing test on Windows, I think because of file paths
  • I confirmed that passing both models and select raises an error (good!), passing both resource_types params still doesn't raise the expected error ("models" and "resource_type" are mutually exclusive arguments). This isn't the most important thing: we're thinking of retiring the --models flag/param soon, anyway (Remove the distinction between --select and --models flags when working with node types #3210).

Once the failing test is sorted, I'm quite happy with where this is at. Nice work!

test/integration/047_dbt_ls_test/test_ls.py Outdated Show resolved Hide resolved
@iknox-fa
Copy link
Contributor Author

  • There's one last failing test on Windows, I think because of file paths
    Fixed!
  • I confirmed that passing both models and select raises an error (good!), passing both resource_types params still doesn't raise the expected error ("models" and "resource_type" are mutually exclusive arguments). This isn't the most important thing: we're thinking of retiring the --models flag/param soon, anyway (Remove the distinction between --select and --models flags when working with node types #3210).
    Fixed!

Once the failing test is sorted, I'm quite happy with where this is at. Nice work!

Copy link
Contributor

@kwigley kwigley left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

lgtm!

@iknox-fa iknox-fa merged commit a565026 into develop May 27, 2021
@iknox-fa iknox-fa deleted the feature/ls_in_RPC branch May 27, 2021 19:42
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.

Add list RPC method
3 participants