-
Notifications
You must be signed in to change notification settings - Fork 7
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
docs: adds DBT concept documentation #111
Conversation
and updates the DBT Extension how-to.
Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
docs/how-tos/dbt_extensions.rst
Outdated
|
||
To change which DBT packages are installed, use the following Tutor variables: | ||
|
||
- **EXTRA_DBT_PACKAGES**: A list of pip dbt packages for Aspects to install. Add your custom ddt packages here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ddt -> dbt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the preferred way to customize dbt is to change the DBT_REPOSITORY
/ DBT_BRANCH
/ DBT_REPOSITORY_PATH
to your dbt project, and have that project make our dbt package a requirement. Is that you how you did it @Ian2012 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can update it to point to the main branch (to be always up to date) but it's better to have a pinned version so we don't introduce breaking changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting, I thought that EXTRA_DBT_PACKAGES
was for adding custom dbt packages, rather than encouraging people to fork aspects-dbt
or run with a different version?
Let me know which way it should read, and I'll update the recommendations here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, EXTRA_DBT_PACKAGES
is a list of python requirement that will be installed before your custom version of aspects-dbt
will be run.
While, you can change your DBT_REPOSITORY and create a little packages.yml
with aspects-dbt
as a package. This will allow you to have all the base functionality of aspects while creating your custom models without forking. See this commit for an example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool, thank you for these details @Ian2012 ! I'll incorporate them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice gotcha, thanks @Ian2012 !
This is looking better: https://docsopenedxorg--111.org.readthedocs.build/projects/openedx-aspects/en/111/how-tos/dbt_extensions.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a couple of nits
* adds details about package profile, dependency versions * changes formatting of pages for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two last little things then feel free to merge, thanks!
docs/how-tos/dbt_extensions.rst
Outdated
|
||
**Step 2. Link to aspects-dbt** | ||
|
||
Aspects charts depend on the transforms in `aspects-dbt`_, so it's important that your ``dbt`` package also installs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aspects charts depend on the transforms in `aspects-dbt`_, so it's important that your ``dbt`` package also installs | |
Aspects charts depend on the transforms in `aspects-dbt`_, so it's important that your ``dbt`` package also installs the same version of `aspects-dbt`_ as your `tutor_contrib_aspects` plugin uses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point -- I updated "step 2" to better illustrate what those package.yml
values should be:
packages: | ||
- git: "https://github.com/openedx/aspects-dbt.git" | ||
revision: v2.2 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should note here that it will also need to contain all of the contents of the aspects-dbt packages.yml too, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it true? I don't think so. I've been using it with only the aspects-dbt
for two other projects and it has shown no issues. I think dependencies are installed recursively
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bmtcril I think @Ian2012 is correct here.
I couldn't find docs that verify it, which I would expect to find if dbt dependencies didn't work like other package dependencies.
I also dug through the dbt packages on dbt hub to find an example. This demo project depends on dbt-codegen, which in turn depends on dbt-utils, but dbt-utils need not be present in the parent's packages.yml.
b54d4b6
to
98da80b
Compare
@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Supporting information
Closes #68