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

Fix: ensure operator execute method is consistent across all execution base subclasses #805

Conversation

jbandoro
Copy link
Collaborator

@jbandoro jbandoro commented Jan 19, 2024

Description

This fixes an issue reported in #804 after the refactor done in #774 where the execute methods for DbtLocalBaseOperator, DbtDockerBaseOperator, and DbtKubernetesBaseOperator were different.

This PR refactors the execute method to the AbstractDbtBaseOperator so it's the same for all of the local, docker and kubernetes inherited operators, and adds build_and_run_cmd as an abstract method since the implementation is different across the 3 different execution modes.

Related Issue(s)

closes #804

Breaking Change?

None

Checklist

  • I have made corresponding changes to the documentation (if required)
  • I have added tests that prove my fix is effective or that my feature works

@jbandoro jbandoro requested a review from a team as a code owner January 19, 2024 22:51
@jbandoro jbandoro requested a review from a team January 19, 2024 22:51
Copy link

netlify bot commented Jan 19, 2024

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit 08bace6
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/65b07a8a4a027b00084f5cbc

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc dbt:run Primarily related to dbt run command or functionality execution:docker Related to Docker execution environment labels Jan 19, 2024
@jbandoro jbandoro changed the title Fix: ensure operator execute method is the same across all operator types Fix: ensure operator execute method is consistent across all execution base subclasses Jan 19, 2024
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ef2c7bb) 94.69% compared to head (08bace6) 94.76%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #805      +/-   ##
==========================================
+ Coverage   94.69%   94.76%   +0.07%     
==========================================
  Files          55       55              
  Lines        2449     2447       -2     
==========================================
  Hits         2319     2319              
+ Misses        130      128       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

It looks great, @jbandoro, thanks for fixing this! It is always exciting to see refactoring and code improvements.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 24, 2024
@tatiana tatiana merged commit 9c090a4 into astronomer:main Jan 24, 2024
42 checks passed
tatiana pushed a commit that referenced this pull request Jan 26, 2024
…ion base subclasses (#805)

This fixes an issue reported in #804 after the refactor done in
#774 where the
`execute` methods for `DbtLocalBaseOperator`, `DbtDockerBaseOperator`,
and `DbtKubernetesBaseOperator` were different.

This PR refactors the `execute` method to the `AbstractDbtBaseOperator`
so it's the same for all of the local, docker and kubernetes inherited
operators, and adds `build_and_run_cmd` as an abstract method since the
implementation is different across the 3 different execution modes.

Closes #804

(cherry picked from commit 9c090a4)
tatiana added a commit that referenced this pull request Jan 26, 2024
Bug fixes

* Fix: ensure DbtGraph.update_node_dependency is called for all load methods by @jbandoro in #803
* Fix: ensure operator execute method is consistent across all execution base subclasses by @jbandoro in #805
* Fix custom selector when test node has no depends_on values by @tatiana in #814
* Fix forwarding selectors to test task when using TestBehavior.AFTER_ALL (#816)

Others

* Docs: Remove incorrect docstring from DbtLocalBaseOperator by @jakob-hvitnov-telia in #797
* Add more logs to troubleshoot custom selector by @tatiana in #809
* Fix OpenLineage integration documentation by @tatiana in #810
* Fix test dependencies after Airflow 2.8 release by @jbandoro and @tatiana in #806
* Use Airflow constraint file for test environment setup by @jbandoro in #812
* pre-commit updates in #799, #807
tatiana added a commit that referenced this pull request Jan 26, 2024
Bug fixes

* Fix: ensure DbtGraph.update_node_dependency is called for all load methods by @jbandoro in #803
* Fix: ensure operator execute method is consistent across all execution base subclasses by @jbandoro in #805
* Fix custom selector when test node has no depends_on values by @tatiana in #814
* Fix forwarding selectors to test task when using TestBehavior.AFTER_ALL (#816)

Others

* Docs: Remove incorrect docstring from DbtLocalBaseOperator by @jakob-hvitnov-telia in #797
* Add more logs to troubleshoot custom selector by @tatiana in #809
* Fix OpenLineage integration documentation by @tatiana in #810
* Fix test dependencies after Airflow 2.8 release by @jbandoro and @tatiana in #806
* Use Airflow constraint file for test environment setup by @jbandoro in #812
* pre-commit updates in #799, #807
tatiana added a commit that referenced this pull request Jan 26, 2024
Bug fixes

* Fix: ensure DbtGraph.update_node_dependency is called for all load methods by @jbandoro in #803
* Fix: ensure operator execute method is consistent across all execution base subclasses by @jbandoro in #805
* Fix custom selector when test node has no depends_on values by @tatiana in #814
* Fix forwarding selectors to test task when using TestBehavior.AFTER_ALL by @tatiana in #816

Others

* Docs: Remove incorrect docstring from DbtLocalBaseOperator by @jakob-hvitnov-telia in #797
* Add more logs to troubleshoot custom selector by @tatiana in #809
* Fix OpenLineage integration documentation by @tatiana in #810
* Fix test dependencies after Airflow 2.8 release by @jbandoro and @tatiana in #806
* Use Airflow constraint file for test environment setup by @jbandoro in #812
* pre-commit updates in #799, #807
@tatiana tatiana mentioned this pull request Jan 26, 2024
tatiana added a commit that referenced this pull request Jan 26, 2024
**Bug fixes**

* Fix: ensure ``DbtGraph.update_node_dependency`` is called for all load
methods by @jbandoro in #803
* Fix: ensure operator ``execute`` method is consistent across all
execution base subclasses by @jbandoro in #805
* Fix custom selector when ``test`` node has no ``depends_on`` values by
@tatiana in #814
* Fix forwarding selectors to test task when using
``TestBehavior.AFTER_ALL`` by @tatiana in #816

**Others**

* Docs: Remove incorrect docstring from ``DbtLocalBaseOperator`` by
@jakob-hvitnov-telia in #797
* Add more logs to troubleshoot custom selector by @tatiana in #809
* Fix OpenLineage integration documentation by @tatiana in #810
* Fix test dependencies after Airflow 2.8 release by @jbandoro and
@tatiana in #806
* Use Airflow constraint file for test environment setup by @jbandoro in
#812
* pre-commit updates in #799, #807
tatiana added a commit that referenced this pull request Jan 26, 2024
Bug fixes

* Fix: ensure DbtGraph.update_node_dependency is called for all load methods by @jbandoro in #803
* Fix: ensure operator execute method is consistent across all execution base subclasses by @jbandoro in #805
* Fix custom selector when test node has no depends_on values by @tatiana in #814
* Fix forwarding selectors to test task when using TestBehavior.AFTER_ALL by @tatiana in #816

Others

* Docs: Remove incorrect docstring from DbtLocalBaseOperator by @jakob-hvitnov-telia in #797
* Add more logs to troubleshoot custom selector by @tatiana in #809
* Fix OpenLineage integration documentation by @tatiana in #810
* Fix test dependencies after Airflow 2.8 release by @jbandoro and @tatiana in #806
* Use Airflow constraint file for test environment setup by @jbandoro in #812
* pre-commit updates in #799, #807
@tatiana tatiana mentioned this pull request Jan 27, 2024
tatiana added a commit that referenced this pull request Jan 27, 2024
**Bug fixes**

* Fix: ensure ``DbtGraph.update_node_dependency`` is called for all load
methods by @jbandoro in #803
* Fix: ensure operator ``execute`` method is consistent across all
execution base subclasses by @jbandoro in #805
* Fix custom selector when ``test`` node has no ``depends_on`` values by
@tatiana in #814
* Fix forwarding selectors to test task when using
``TestBehavior.AFTER_ALL`` by @tatiana in #816

**Others**

* Docs: Remove incorrect docstring from ``DbtLocalBaseOperator`` by
@jakob-hvitnov-telia in #797
* Add more logs to troubleshoot custom selector by @tatiana in #809
* Fix OpenLineage integration documentation by @tatiana in #810
* Fix test dependencies after Airflow 2.8 release by @jbandoro and
@tatiana in #806
* Use Airflow constraint file for test environment setup by @jbandoro in
#812
* pre-commit updates in #799, #807

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Justin Bandoro <[email protected]>
Co-authored-by: Jakob Aron Hvitnov <[email protected]>
arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this pull request Jul 14, 2024
…ion base subclasses (astronomer#805)

This fixes an issue reported in astronomer#804 after the refactor done in
astronomer#774 where the
`execute` methods for `DbtLocalBaseOperator`, `DbtDockerBaseOperator`,
and `DbtKubernetesBaseOperator` were different.

This PR refactors the `execute` method to the `AbstractDbtBaseOperator`
so it's the same for all of the local, docker and kubernetes inherited
operators, and adds `build_and_run_cmd` as an abstract method since the
implementation is different across the 3 different execution modes.

Closes astronomer#804
arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this pull request Jul 14, 2024
**Bug fixes**

* Fix: ensure ``DbtGraph.update_node_dependency`` is called for all load
methods by @jbandoro in astronomer#803
* Fix: ensure operator ``execute`` method is consistent across all
execution base subclasses by @jbandoro in astronomer#805
* Fix custom selector when ``test`` node has no ``depends_on`` values by
@tatiana in astronomer#814
* Fix forwarding selectors to test task when using
``TestBehavior.AFTER_ALL`` by @tatiana in astronomer#816

**Others**

* Docs: Remove incorrect docstring from ``DbtLocalBaseOperator`` by
@jakob-hvitnov-telia in astronomer#797
* Add more logs to troubleshoot custom selector by @tatiana in astronomer#809
* Fix OpenLineage integration documentation by @tatiana in astronomer#810
* Fix test dependencies after Airflow 2.8 release by @jbandoro and
@tatiana in astronomer#806
* Use Airflow constraint file for test environment setup by @jbandoro in
astronomer#812
* pre-commit updates in astronomer#799, astronomer#807

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Justin Bandoro <[email protected]>
Co-authored-by: Jakob Aron Hvitnov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc dbt:run Primarily related to dbt run command or functionality execution:docker Related to Docker execution environment lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DbtRunOperationKubernetesOperator lost --args in 1.3.1
2 participants