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 selected_resources to the Jinja context #5001

Merged
merged 7 commits into from
Apr 12, 2022

Conversation

b-per
Copy link
Contributor

@b-per b-per commented Apr 6, 2022

resolves #3471

Description

I don't know if I followed the correct approach but this code is adding a variable called selected_resources to the Jinja context. This is a list of all models/tests/snapshots etc... captured by the selectors for the dbt run. I checked how flags were added to the context and did the same for selected_resources, hence why it is under the core/dbt folder.

The issue #3471 also mentions making this available for run-operation but this will be taken care of in another issue.

My testing approach is to use a on-run-start hook to compare the value of selected_resources with the expected one.

If this is merged the docs will need to be updated to mention the new variable and highlight that similar to graph is will only contain some information at execute time.

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 added information about my change to be included in the CHANGELOG.

@b-per b-per requested a review from a team April 6, 2022 12:04
@b-per b-per requested review from a team as code owners April 6, 2022 12:04
@b-per b-per requested review from ChenyuLInx and gshank April 6, 2022 12:04
@cla-bot cla-bot bot added the cla:yes label Apr 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

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.

@@ -556,6 +557,15 @@ def flags(self) -> Any:
"""
return flags

@contextproperty
def selected_resources(self) -> Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure that the base context is the wrong context for this functionality. We don't need this in yaml rendering and other non-sql rendering. It probably ought to go in a provider context. Is there some reason that you put it in base?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it in Base because I looked at howflags were configured and replicated some of the logic for selected_resources, but I get your point and it makes much more sense to be in another context. I will update the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have just moved it to the ProviderContext

gshank
gshank previously requested changes Apr 6, 2022
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

Looks like you need to update the tests. In test_context.py you need to add 'selected_resources' to REQUIRED_MACRO_KEYS.

Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

LGTM with one nitpicking item for tests. I feel like this is ready to go once other tests are fixed.

@@ -0,0 +1,35 @@
on_run_start_macro_assert_selected_models_expected_list = """
{% macro assert_selected_models_expected_list(expected_list) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Really like the way you use this macro to test the functionality!

[
"build",
"--select",
"model1+",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do a model2+ as one of the test cases? since the result of this selection is the same as the no select specified test below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just modified a test to be model2+

It was failing at first (because model1 was not built) so I moved all the tests to a Class that uses the fixture build_all, meaning that a full dbt build is run before each test (which allows us to select nodes that are depending on other ones)

Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

LGTM! Left one question but not a blocker

@@ -0,0 +1,9 @@
kind: Features
body: Add a variable called selected_resources in the Jinja context containing a list
Copy link
Contributor

Choose a reason for hiding this comment

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

@gshank could there be situations where selected_resources is referenced before it got set by get_graph_queue ? I think there isn't but want to double-check.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the selected_resources will be empty unless it's execution time, which should be okay. I suppose the list could be really big for a run command that executes the whole project, but I think that should be okay too; it doesn't do much except reference the list of nodes.

@ChenyuLInx ChenyuLInx self-requested a review April 12, 2022 16:23
@ChenyuLInx ChenyuLInx dismissed gshank’s stale review April 12, 2022 16:25

the test has been updated

@ChenyuLInx ChenyuLInx merged commit 15ad34e into main Apr 12, 2022
@ChenyuLInx ChenyuLInx deleted the feature/3471-add-selected_resources-to-context branch April 12, 2022 16:25
jtcohen6 added a commit to dbt-labs/dbt-redshift that referenced this pull request Apr 13, 2022
jtcohen6 added a commit to dbt-labs/dbt-redshift that referenced this pull request Apr 13, 2022
leahwicz added a commit to dbt-labs/dbt-redshift that referenced this pull request Apr 13, 2022
* Bumping version to 1.1.0rc1

* Updating requirements to latest release branch

* Bumping manifest schema in tests

* Fix unit test per dbt-labs/dbt-core#5001

Co-authored-by: Github Build Bot <[email protected]>
Co-authored-by: leahwicz <[email protected]>
Co-authored-by: Jeremy Cohen <[email protected]>
jtcohen6 added a commit to dbt-labs/dbt-redshift that referenced this pull request Apr 13, 2022
* Bumping version to 1.2.0a1

* Bumping schema version in tests

* Try skipping test-build if alpha version

* try this

* i meant this

* Fix unit test per dbt-labs/dbt-core#5001

* One less change than needed

Co-authored-by: Github Build Bot <[email protected]>
Co-authored-by: leahwicz <[email protected]>
VersusFacit pushed a commit that referenced this pull request Apr 14, 2022
* Add selected_resources in the Jinja context

* Add tests for the Jinja variable selected_resources

* Add Changie entry for the addition of selected_resources

* Move variable to the ProviderContext

* Move selected_resources from ModelContext to ProviderContext

* Update unit tests for context to cater for the new selected_resources variable

* Move tests to a Class where tests are run after a dbt build
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request May 20, 2022
* Add selected_resources in the Jinja context

* Add tests for the Jinja variable selected_resources

* Add Changie entry for the addition of selected_resources

* Move variable to the ProviderContext

* Move selected_resources from ModelContext to ProviderContext

* Update unit tests for context to cater for the new selected_resources variable

* Move tests to a Class where tests are run after a dbt build
abbywh pushed a commit to abbywh/dbt-redshift that referenced this pull request Oct 11, 2023
* Bumping version to 1.2.0a1

* Bumping schema version in tests

* Try skipping test-build if alpha version

* try this

* i meant this

* Fix unit test per dbt-labs/dbt-core#5001

* One less change than needed

Co-authored-by: Github Build Bot <[email protected]>
Co-authored-by: leahwicz <[email protected]>
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.

Jinja context variable for selected resources
4 participants