-
Notifications
You must be signed in to change notification settings - Fork 137
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
Run dbt deps before running the dbt ls command #481
Run dbt deps before running the dbt ls command #481
Conversation
👷 Deploy Preview for amazing-pothos-a3bca0 processing.
|
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.
This looks great. Thanks a lot for your first contribution, @DanMawdsleyBA ! I'm hopeful this could be part of the upcoming 1.1 release.
I can see two areas for improvement:
- A test that represents the original issue this PR solves, that breaks before the change and passes after this modification. Ideally, it would highlight what happens when a profile depends on a specific dbt adapter that is not installed and which error would be raised with
dbt ls
. - Do you think all users would be happy for, during DAG parsing time, to have
dbt deps
installed? What would happen if they had a read-only filesystem? A possibility to address this could be to have a new parameter added toRenderConfig
, something likeinstall_deps
, set toTrue
by default, but users could opt out (@jlaneve may have other ideas/suggestions)
Please do reach out if you'd like support writing the test.
@tatiana |
One thing I have realised is that this fix won't work on systems that have the read only issue as by default the packages are stored in dbt project path/ packages . There is a variable that can be set in the dbt_project.yml |
Hi @DanMawdsleyBA , that's a very important point. |
f484d49
to
f2c06d7
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #481 +/- ##
==========================================
- Coverage 91.59% 91.51% -0.08%
==========================================
Files 50 50
Lines 1856 1874 +18
==========================================
+ Hits 1700 1715 +15
- Misses 156 159 +3
☔ View full report in Codecov by Sentry. |
@DanMawdsleyBA We managed to get all the tests to pass 🎉 Let's see after the rebase. Once this is passing, we'll merge this. It will be released as part of 1.1. |
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.
Happy for these changes to be merged, I could test it locally.
It will help many Cosmos users with DAGs with no nodes due to dependencies not being installed. We'll improve the coverage in a follow up PR.
**Features** * Support dbt global flags (via dbt_cmd_global_flags in operator_args) by @tatiana in #469 * Support parsing DAGs when there are no connections by @jlaneve in #489 **Enhancements** * Hide sensitive field when using BigQuery keyfile_dict profile mapping by @jbandoro in #471 * Consistent Airflow Dataset URIs, inlets and outlets with `Openlineage package <https://pypi.org/project/openlineage-integration-common/>`_ by @tatiana in #485. `Read more <https://astronomer.github.io/astronomer-cosmos/configuration/lineage.html>`_. * Refactor ``LoadMethod.DBT_LS`` to run from a temporary directory with symbolic links by @tatiana in #488 * Run ``dbt deps`` when using ``LoadMethod.DBT_LS`` by @DanMawdsleyBA in #481 * Update Cosmos log color to purple by @harels in #494 * Change operators to log ``dbt`` commands output as opposed to recording to XCom by @tatiana in #513 **Bug fixes** * Fix bug on select node add exclude selector subset ids logic by @jensenity in #463 * Refactor dbt ls to run from a temporary directory, to avoid Read-only file system errors during DAG parsing, by @tatiana in #414 * Fix profile_config arg in DbtKubernetesBaseOperator by @david-mag in #505 * Fix SnowflakePrivateKeyPemProfileMapping private_key reference by @nacpacheco in #501 * Fix incorrect temporary directory creation in VirtualenvOperator init by @tatiana in #500 * Fix log propagation issue by @tatiana in #498 * Fix PostgresUserPasswordProfileMapping to retrieve port from connection by @jlneve in #511 **Others** * Docs: Fix RenderConfig load argument by @jbandoro in #466 * Enable CI integration tests from external forks by @tatiana in #458 * Improve CI tests runtime by @tatiana in #457 * Change CI to run coverage after tests pass by @tatiana in #461 * Fix forks code revision in code coverage by @tatiana in #472 * [pre-commit.ci] pre-commit autoupdate by @pre-commit-ci in #467 * Drop support to Python 3.7 in the CI test matrix by @harels in #490 * Add Airflow 2.7 to the CI test matrix by @tatiana in #487 * Add MyPy type checks to CI since we exceeded pre-commit disk quota usage by @tatiana in #510
Description
Runs the dbt deps command before running the dbt ls command
Related Issue(s)
Closes #445
Breaking Change?
Checklist