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

All-around improvements to 0.18.0 docs #363

Merged
merged 9 commits into from
Sep 3, 2020
Merged

All-around improvements to 0.18.0 docs #363

merged 9 commits into from
Sep 3, 2020

Conversation

jtcohen6
Copy link
Collaborator

@jtcohen6 jtcohen6 commented Aug 28, 2020 •
edited
Loading

Description & motivation

  • Split out node selection syntax into several docs, with renames + redirects
  • Add "best practice" for slim CI
  • Net-new docs (additions to existing pages) for already merged, new-in-v0.18.0 features:
    • full_refresh node config
    • BigQuery: policy_tags, hours_to_expiration
    • Snowflake: query_tag
  • Tiny reorg to docs on BQ service account impersonation

Pre-release docs

Is this change related to an unreleased version of dbt?

  • Yes: please update the base branch to next
  • No: please ensure the base branch is current
  • Unsure: we'll let you know!

Checklist

If you added new pages (delete if not applicable):

  • The page has been added to website/sidebars.js
  • The new page has a unique filename

If you removed existing pages (delete if not applicable):

  • The page has been removed from website/sidebars.js
  • An entry has been added to _redirects

drewbanin
drewbanin reviewed Aug 31, 2020
View reviewed changes
website/docs/reference/node-selection/yaml-selectors.md
selectors:
- name: nodes_to_joy
definition: ...
- name: nodes_to_a_grecian_urn
Copy link
Collaborator

Choose a reason for hiding this comment

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

community.getdbt.com/how-to-write-high-quality-documentation#using-obscure-greek-references

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all you need to know

@jtcohen6 jtcohen6 marked this pull request as ready for review September 1, 2020 13:35
@jtcohen6 jtcohen6 requested a review from clrcrl as a code owner September 1, 2020 13:35
clrcrl
clrcrl suggested changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@clrcrl clrcrl left a comment

Choose a reason for hiding this comment

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

Split out node selection syntax into several docs, with renames + redirects

Very supportive of the concept behind change! I think we can do even more here, e.g. I am a fan of how the test selection docs have very discrete example — one day we should get to that level for all node types.

A note to discuss: I think I've previously used the word "resource" where we are now using "node". We should aim to be consistent at some point, all the way down to making this consistent:

dbt ls --resource-type models

I also left a suggestion for a formatting error.

The rest of the commits

LGTM!

website/docs/reference/node-selection/methods.md Outdated
Comment on lines 51 to 52
<Changelog>New in v0.18.0</Changelog>
The `config` method is used to select models that match a specified [node config](config).
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not rendering correctly:

Suggested change
<Changelog>New in v0.18.0</Changelog>
The `config` method is used to select models that match a specified [node config](config).
<Changelog>New in v0.18.0</Changelog>
The `config` method is used to select models that match a specified [node config](config).

And so on and so forth through the rest of this page

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ooh nice catch! TIL

Copy link
Contributor

Choose a reason for hiding this comment

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

don't ask me why — i do not know

@jtcohen6
Copy link
Collaborator Author

jtcohen6 commented Sep 1, 2020

Thanks for the review @clrcrl!

I realize that I've been using node and resource interchangeably, at some points using one as the definition of the other. ("Each dbt resource becomes a node in the DAG.") I agree that we should pick one, ultimately; for now I've just added a callout that, in fact, they mean the same thing.

drewbanin
drewbanin approved these changes Sep 2, 2020
View reviewed changes
Copy link
Collaborator

@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.

This is all amazing - approved on my end :D

@clrcrl clrcrl self-requested a review September 2, 2020 19:18
clrcrl
clrcrl approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@clrcrl clrcrl left a comment

Choose a reason for hiding this comment

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

:shipit:

@jtcohen6 jtcohen6 merged commit b14110a into next Sep 3, 2020
@jtcohen6 jtcohen6 deleted the feature/better-0180-docs branch September 3, 2020 12:53
@jtcohen6 jtcohen6 mentioned this pull request Sep 3, 2020
Merged
nghi-ly pushed a commit that referenced this pull request Feb 13, 2024
@john-rock
Merge pull request #363 from dbt-labs/repo-sync
4817861 
REPO SYNC - Public to Private
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clrcrl clrcrl clrcrl approved these changes

@drewbanin drewbanin drewbanin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jtcohen6 @drewbanin @clrcrl