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

Allow ref override #2028

Merged

Conversation

alanmcruickshank
Copy link
Contributor

This is a very late re-entry, but the bug has hit me again and I thought I should get round to resubmitting a fox for #1603 .

These edits allow users to override built in functions like ref.

For our use case (where we want to stop rendering the database portion of the object reference), the user just needs to have a macro such as:

ref_override.sql:

{% macro ref(modelname) %}{{ builtins.ref(modelname).include(database=False).render() }}{% endmacro %}

And the ref function will be overridden, now not rendering the database.

@drewbanin - I think this is much closer to what you originally envisioned. Have I overridden the parts that you had originally intended? If this is in line, I'd like to add the example macro above somewhere in the documentation for other users to find. Where would the best place be?

@cla-bot
Copy link

cla-bot bot commented Jan 3, 2020

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @alanmcruickshank

@alanmcruickshank
Copy link
Contributor Author

@drewbanin - I've now signed the CLA.

@drewbanin
Copy link
Contributor

Thanks for making this PR @alanmcruickshank! I think this looks really good - I'll do some functional testing now. Adding @beckjake to review since he's spent a lot of time around this part of the codebase recently.

This probably won't make it out for 0.15.1 - can you change the base branch to dev/barbara-gittings? Thanks also for signing the CLA :) @cla-bot check

@cla-bot cla-bot bot added the cla:yes label Jan 5, 2020
@cla-bot
Copy link

cla-bot bot commented Jan 5, 2020

The cla-bot has been summoned, and re-checked this pull request!

Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

This looks great! I have a few suggestions on minor code style tweaks, but I love the idea and the general concept of the implementation. Let me know if I missed any obvious reasons for doing things this way!

Also, can you please add an integration test exercising this feature (postgres is fine as this is very generic code, feel free to do repeated pushes to trigger the azure postgres tests). I've been doing some contexts work elsewhere and I would hate to cause a regression. I'd be happy to help you with writing one if you need assistance, I know the test suite can be pretty arcane.

core/dbt/context/common.py Outdated Show resolved Hide resolved
core/dbt/context/common.py Outdated Show resolved Hide resolved
core/dbt/context/base.py Outdated Show resolved Hide resolved
@alanmcruickshank
Copy link
Contributor Author

Thanks @drewbanin @beckjake .

  • Rebased onto dev/barbara-gittings.
  • Added changes from code review. All the suggestions were very sensible. Thanks @beckjake.
  • I'd love to write some integration tests around this too. Last time I tried I found it really hard to work out how to write tests for the rendering of templates. @beckjake is there an existing test that would be a good starting point to write some tests for this? With an existing example and a couple of pointers, I'm happy to do the rest of the legwork.

@beckjake
Copy link
Contributor

beckjake commented Jan 5, 2020

@alanmcruickshank My method for writing a new integration test is usually this:

  1. make a new folder with ${test_folder_number}_${somewhat_useful_name} under test/integration, where test_folder_number=${biggest_existing_number} + 1 - in this case, probably something like 055_ref_override_test. It doesn't have to be great, but I should be able to figure out what kind of tests are in there by reading it.
  2. Make a new file in that folder named test_${something}.py. test_ref_override.py is probably fine.
  3. Put something following this pattern in there:
from test.integration.base import DBTIntegrationTest, use_profile


class TestRefOverride(DBTIntegrationTest):
    @property
    def schema(self):
        return "${some_name}_${test_folder_number}"

    # you'll need this if you're using seeds/data tests in your test
    @property
    def project_config(self):
        return {
            'data-paths': ['data'],
            'test-paths': ['tests'],
        }

    @property
    def models(self):
        return "models"

    @use_profile('postgres')
    def test_postgres_ref_override(self):
        self.run_dbt(['seed']) #if this is how you're testing
        self.run_dbt(['run'])
        self.run_dbt(['test']) #if this is how you're testing
        self.assertTablesEqual('seed_model', 'my_model')

The test should have _postgres_ in the method name and have the @use_profile('postgres') decorator. If you don't have both, the test suite will probably complain. By default, run_dbt asserts that dbt ${cmd} exited successfully.

  1. Make a models directory in your test folder, and put any relevant test models in there that exercise your change, and potentially any tests that match it in a schema.yml file. You can also add macros and data directories alongside that contain macros (I assume you'll need that!) or seeds. Similarly, tests if you need data tests.

For the test itself, there are broadly two techniques:

  • have a seed that creates a table you want, and assert that the model dbt seed creates is equal to the model dbt run creates, using self.assertTablesEqual(). Instead of using dbt seed you can write a seed sql file that you execute with self.run_sql_file() as part of the test, but tests using dbt seed are usually easier to maintain/migrate to new databases.
  • have a schema test/data test that asserts your run did the right thing.

009_data_tests_test is a pretty god example of the second technique, I guess 005_simple_seed_test is a decent example of the first technique (see the test_column_type macro). A lot of tests kind of blend the two methods.

@alanmcruickshank alanmcruickshank changed the base branch from dev/0.15.1 to dev/barbara-gittings January 6, 2020 13:16
@alanmcruickshank alanmcruickshank force-pushed the ref_override branch 2 times, most recently from 3f763e3 to a643d5c Compare January 6, 2020 13:21
@beckjake
Copy link
Contributor

beckjake commented Jan 6, 2020

I should have mentioned: I haven't set it up fresh in quite a while, but if you don't want to push to trigger tests, and you have docker/docker-compose set up, you should be able to run your test locally with something like this:

cp test.env.sample test.env
docker-compose run --rm test tox -e explicit-py36 -- -s -x -m profile_postgres test/integration/055_ref_override_test

The first time will probably take a while as it has to pull some big silly containers (postgres + our test one) and build the tox environment.

@alanmcruickshank
Copy link
Contributor Author

@beckjake - integration test is in and working - thanks for the pointers! Yes it did take a while to download but still faster than the azure tests 😄 .

I've squashed all my commits for tidiness. Ready for review I think.

@drewbanin
Copy link
Contributor

just kicked off the tests here @alanmcruickshank :)

Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

Looks good to me! Let's just see how the tests go (thanks for kicking them off @drewbanin)

@drewbanin
Copy link
Contributor

Really nice work @alanmcruickshank! I think we'll want to update the docs here: https://docs.getdbt.com/docs/jinja-context

We can add a section for builtins and show an example that uses this macro under the ref entry. My understanding is that you wanted this feature for a sort of blue/green deployment scheme, right? It would be awesome to write up a guide on how to do something like that, then link out to these docs for the specifics.

I usually handle the docs updates when a new release is ready, but let me know if there are specific things you'd like to touch on and I can be sure to include them.

Merging this! Thanks again for this really lovely PR :)

@drewbanin drewbanin merged commit 4ed1986 into dbt-labs:dev/barbara-gittings Jan 6, 2020
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.

3 participants