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

Feature/0.18 node selectors #118

Merged
merged 5 commits into from
Aug 3, 2020
Merged

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented Aug 2, 2020

fixes #116

The DAG view now supports:

  • selecting my config, test_type, test_name, package
  • node intersection syntax
  • arbitrary parent/child depth

This PR does not support YML-based selectors

Additionally, I added a bunch of graph-related tests and hooked them up to run in netlify on push

@drewbanin drewbanin requested a review from jtcohen6 August 2, 2020 22:02
@@ -192,7 +192,7 @@ <h6>
class="field-input form-control input-dark"
ng-model="selectorService.selection.dirty.include"
placeholder="..." />
<div class="field-label">--models</div>
<div class="field-label">--select</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: this changes the --models label to --select, which feels appropriate if not 100% accurate all of the time. Probably something we should give more thought to for a future iteration of this UI

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this change. It feels right to have the DAG mirror the behavior and syntax of dbt ls. And as Jake pointed out to me (not long ago), dbt ls --models ... is basically dbt ls --resource-type model --select.

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.

heroic

  • intersections ✅
  • nth parent/child ✅
  • config, package method ✅
  • test_name, test_type ❔
    • test_type:data works fine
    • test_name is exclusive to schema tests (implicitly, I think, by requiring test_metadata)
    • separate conversation, not for this PR: When (if ever) should the DAG include schema test nodes?

This PR does not support YML-based selectors

Is this for another PR, or do you have a feeling against supporting YML-defined selectors in the DAG view?

I sort of follow how the tests run in netlify: if netlify runs make dist-ci, and the deployment succeeded, and the tests run before webpack, the tests must also have succeeded. In any case, I ran them locally, and they look good.

@@ -192,7 +192,7 @@ <h6>
class="field-input form-control input-dark"
ng-model="selectorService.selection.dirty.include"
placeholder="..." />
<div class="field-label">--models</div>
<div class="field-label">--select</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this change. It feels right to have the DAG mirror the behavior and syntax of dbt ls. And as Jake pointed out to me (not long ago), dbt ls --models ... is basically dbt ls --resource-type model --select.

@drewbanin
Copy link
Contributor Author

separate conversation, not for this PR: When (if ever) should the DAG include schema test nodes?

Yeah... i kind of punted on that question. I think adding them as "nodes" in the DAG might not be quite right, but i do think there should be a way to visualize tests on nodes in the DAG view? Definitely one to ponder.

Is this for another PR, or do you have a feeling against supporting YML-defined selectors in the DAG view?

We can totally do that in a separate PR... we should sync up about how to make that happen for the 0.18.0 release

re: tests: yeah, i'm just using the makefile here. We should probably set this up with a circle workflow than run tests before kicking off the netlify build. Low impact low priority at the moment IMO :D

@drewbanin drewbanin merged commit cefda62 into master Aug 3, 2020
@drewbanin drewbanin deleted the feature/0.18-node-selectors branch August 3, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New node selection syntax in DAG
2 participants