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

Tests for source overriding (#2264) #2291

Merged
merged 3 commits into from
Apr 16, 2020

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Apr 2, 2020

resolves #2264

Description

  • Make sure dependency sources can be selected from the root project
  • Make sure the root project can override dependency sources

Is there more we want to assert on here?

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.

- Make sure dependency sources can be selected from the root project
- Make sure the root project can override dependency sources
@cla-bot cla-bot bot added the cla:yes label Apr 2, 2020
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

Some other things we could test:

  • The list of columns for a source in local_models should override the list of columns in the dependency package for the same source. This would be most noticeable if there were column tests defined in the dependency package that should be overridden in the root project. This behavior might currently be poorly defined. I think this case, we'd just want to clobber the list of columns rather than deep-merge them.
  • Tags from the root project for the source should override tags defined in the dependency package. I could buy that these tags should be extended, but I think it would probably be more consistent to clobber them instead. What do you think?
  • Source-level quoting configs from the root project should override source-level quoting configs from the dependency package.

Can you think of any others? We don't need to test every single possible source config, but I'd like to be able to describe and document dbt's exact behavior here with confidence!

CHANGELOG.md Outdated Show resolved Hide resolved
@beckjake
Copy link
Contributor Author

beckjake commented Apr 7, 2020

The list of columns for a source in local_models should override the list of columns in the dependency package for the same source. This would be most noticeable if there were column tests defined in the dependency package that should be overridden in the root project. This behavior might currently be poorly defined. I think this case, we'd just want to clobber the list of columns rather than deep-merge them.

This doesn't play well with how dbt behaves. Since the two sources both show up in the manifest as distinct entities (they're in different packages), test selection ends up selecting the tests for each source. I think the easiest fix to this to have test selection take care of this, but that feels all weird. If you want sources to truly overwrite, that's going to be quite the architectural change.

@beckjake
Copy link
Contributor Author

beckjake commented Apr 7, 2020

Tags from the root project for the source should override tags defined in the dependency package. I could buy that these tags should be extended, but I think it would probably be more consistent to clobber them instead. What do you think?

Similar to the above concern: What currently happens is both sources exist in the manifest and have their own lists of tags. So you kind of get both.

@beckjake
Copy link
Contributor Author

beckjake commented Apr 7, 2020

Source-level quoting configs from the root project should override source-level quoting configs from the dependency package.

Keeping with the theme here - currently each source config gets its own quoting config based on its package...

@beckjake
Copy link
Contributor Author

beckjake commented Apr 7, 2020

It kind of feels like what you want here is for sources to not have their package name in their unique ID, and therefore sources are sort of "communal" and each source table can override others in dependency packages... Is that dumb?

Co-Authored-By: Drew Banin <drew@fishtownanalytics.com>
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

Let's merge this as-is -- I'll open a new issue for the types of changes we've discussed inline here

@beckjake beckjake merged commit 204fc25 into dev/octavius-catto Apr 16, 2020
@beckjake beckjake deleted the feature/test-source-overrides-deps branch April 16, 2020 16:57
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.

Tests for source overriding
2 participants