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.

Sign up for GitHub

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

Jump to bottom

Compile assets as part of docs generate #2623

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Compile assets as part of docs generate #2623

merged 1 commit into from
Jul 28, 2020

Conversation

clrcrl
Copy link
Contributor

@clrcrl clrcrl commented Jul 13, 2020 •
edited
Loading

resolves #2072

Description

When a user runs dbt docs generate, any files in their assets directory (configurable!) will be compiled to the target/assets directory — useful for adding images to documentation

Extra validation

I added an image to my own project, at assets/dbt-logo.png, and then added it as a description:

version: 2

models:
  - name: customers
    description: "![dbt Logo](assets/dbt-logo.png)"

It totally works 😎
Screen Shot 2020-07-21 at 12 53 05 PM

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 Jul 13, 2020
@clrcrl clrcrl requested a review from beckjake July 13, 2020 13:30
@beckjake
Copy link
Contributor

Is having this as part of docs generate a reasonable solution?

Sure, I don't see why not.

Should I have a test that checks for the presence of a compiled file? If so, can you point me to any other tests that assert that a file exists?

Yeah. Check out the 040_init_test integration tests.

beckjake
beckjake reviewed Jul 13, 2020
View reviewed changes
test/unit/test_config.py Outdated Show resolved Hide resolved
@clrcrl clrcrl force-pushed the asset-paths branch 2 times, most recently from f5065fd to 6e4e9db Compare July 21, 2020 14:51
clrcrl
clrcrl commented Jul 21, 2020
View reviewed changes
core/dbt/config/project.py Outdated
@@ -401,6 +402,7 @@ def from_project_config(
)

docs_paths: List[str] = value_or(cfg.docs_paths, all_source_paths)
asset_paths: List[str] = value_or(cfg.asset_paths, ['assets'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

heads up, this differs from the original ticket that had the default as ['static'], but I prefer the consistency

@clrcrl clrcrl changed the title WIP: Compile assets as part of docs generate Compile assets as part of docs generate Jul 21, 2020
@clrcrl clrcrl marked this pull request as ready for review July 21, 2020 17:10
@clrcrl clrcrl requested a review from beckjake July 21, 2020 17:10
beckjake
beckjake approved these changes Jul 21, 2020
View reviewed changes
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 good to me, great tests!

The only question I have is: should this default to ['assets'], or to []? Maybe you should have to opt-in to have dbt copy your files around? @jtcohen6 I guess this question is for you

clrcrl
clrcrl commented Jul 21, 2020
View reviewed changes
CHANGELOG.md Outdated
@@ -9,6 +9,7 @@
- Add more helpful error message for misconfiguration in profiles.yml ([#2569](https:/fishtown-analytics/dbt/issues/2569), [#2627](https:/fishtown-analytics/dbt/pull/2627))
### Fixes
- Adapter plugins can once again override plugins defined in core ([#2548](https:/fishtown-analytics/dbt/issues/2548), [#2590](https:/fishtown-analytics/dbt/pull/2590))
- Compile assets as part of docs generate ([#2072](https:/fishtown-analytics/dbt/issues/2072), [#2623](https:/fishtown-analytics/dbt/pull/2623/files#))
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'll fix this lol

Suggested change
- Compile assets as part of docs generate ([#2072](https:/fishtown-analytics/dbt/issues/2072), [#2623](https:/fishtown-analytics/dbt/pull/2623/files#))
- Compile assets as part of docs generate ([#2072](https:/fishtown-analytics/dbt/issues/2072), [#2623](https:/fishtown-analytics/dbt/pull/2623))

@clrcrl
Copy link
Contributor Author

clrcrl commented Jul 21, 2020

Also @drewbanin — can you add your comments re: whether this should work in dbt Cloud?

@jtcohen6
Copy link
Contributor

The only question I have is: should this default to ['assets'], or to []? Maybe you should have to opt-in to have dbt copy your files around? @jtcohen6 I guess this question is for you

Great point, I think this should be opt-in. The majority of users shouldn't need to worry about an assets folder (or even know such a thing exists) until they need it and read the docs about it, at which point adding an extra config line to dbt_project.yml feels appropriate. I'd hate to have someone accidentally add an entirely unrelated root-level directory named assets and be surprised.

@clrcrl
Copy link
Contributor Author

clrcrl commented Jul 21, 2020 •
edited
Loading

I was going to argue the counter point — every thing else has a default directory, so we should be consistent here.

But, turns out that analyses don't have a default directory! TIL!

@clrcrl clrcrl requested a review from drewbanin July 23, 2020 18:34
@clrcrl
Copy link
Contributor Author

clrcrl commented Jul 23, 2020

@drewbanin — assigning you since you have context about the potential security implications of this PR

@clrcrl clrcrl removed the request for review from drewbanin July 28, 2020 19:44
@clrcrl
Copy link
Contributor Author

clrcrl commented Jul 28, 2020

Just rebased and pushed :)

@clrcrl clrcrl merged commit c9eb3c2 into dev/marian-anderson Jul 28, 2020
@clrcrl clrcrl deleted the asset-paths branch July 28, 2020 21:15
@clrcrl clrcrl mentioned this pull request Jul 30, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beckjake beckjake beckjake approved these changes

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compile arbitrary files in target directory
3 participants
@clrcrl @beckjake @jtcohen6