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

Use original path to mimick resource subdirectory structure #2349

Merged
merged 11 commits into from
May 4, 2020

Conversation

franloza
Copy link
Contributor

@franloza franloza commented Apr 21, 2020

Resolves #2173

Description

I'm using the original path when compiling to preserve the original folder structure.

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 Apr 21, 2020
@drewbanin
Copy link
Contributor

Thanks for opening this PR @franloza!

It looks like the tests failed here -- there's going to be a bunch of changes to make in the test suite to make actual == expected for these changes. Before you go off an do all of that work, it would be good to have @beckjake take a look at your approach :)

@drewbanin drewbanin requested a review from beckjake April 21, 2020 23:39
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.

I think this idea is generally good. One problem is going to be on schema tests: every test declared in a given file is going to end up with a build_path attribute pointing to the same schema.yml path. You'll see similar issues on snapshots, and anywhere else that has a many-to-one relationship of nodes to files.

One idea is to have test calculate a similar full_path, but make schema.yml a folder and add (at least some of) the test FQN or unique ID to the end. I'm not sure that's the best way, but at least it'll be consistent.

@franloza
Copy link
Contributor Author

@beckjake I have reproduced the error and the approach I have followed to solve the problem in 8f45ece is very similar to the one you have proposed.

If the basename of the path is equal, it means that is one-to-one relationship and we can use the original path safely. However, if the base name is not equal, it means that it is a many-to-one relationship and in that case, we need to consider the basename as a folder (like you proposed to do with schema.yml) and append the path to the original path.

Therefore, for the analysis file, the resulting full path would be target/compiled/customer_data_model/test_paths/test.sql whereas for a custom schema test would be target/compiled/customer_data_model/models/base/schema.yml/schema_test/custom_base_customers_name.sql .

What do you think about this approach? Do you think it is consistent with the original solution?

@beckjake
Copy link
Contributor

@franloza Yeah, that sounds great!

@franloza franloza changed the title WIP: Use original path to mimick resource subdirectory structure Use original path to mimick resource subdirectory structure May 1, 2020
@franloza franloza requested a review from beckjake May 1, 2020 10:30
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.

Thanks for the PR @franloza, this looks great! I'll kick off the tests here.

@beckjake
Copy link
Contributor

beckjake commented May 4, 2020

I'm merging this before these tests finish - this was all passing and the only conflict was in the changelog, which I've resolved.

@beckjake beckjake merged commit bd9a132 into dbt-labs:dev/octavius-catto May 4, 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.

compiled "analysis" code writing to wrong target directory
3 participants