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

Refactor common executor constructors with test coverage #774

Merged

Conversation

jbandoro
Copy link
Collaborator

Description

I noticed in #771 that there was a lot of repeated class constructors in order to add a new execution mode that is common among local, docker and kubernetes and there is no test coverage for the constructors and methods in some of the operators.

This PR attempts to make it easier to add new execution operators in the future.

Related Issue(s)

None

Breaking Change?

None

There may be task UI color differences with the kuberentes/docker operators, since now all of LS/Seed/Run etc. operators across execution modes have the same task colors.

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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@jbandoro jbandoro requested a review from a team as a code owner December 23, 2023 00:17
@jbandoro jbandoro requested a review from a team December 23, 2023 00:17
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Dec 23, 2023
Copy link

netlify bot commented Dec 23, 2023

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

Name Link
🔨 Latest commit 3bc4d88
🔍 Latest deploy log https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/65862935c3eefd0008fe2483

@dosubot dosubot bot added 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 Dec 23, 2023
Copy link

codecov bot commented Dec 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2ac4264) 93.28% compared to head (4997eda) 94.62%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #774      +/-   ##
==========================================
+ Coverage   93.28%   94.62%   +1.34%     
==========================================
  Files          55       55              
  Lines        2502     2419      -83     
==========================================
- Hits         2334     2289      -45     
+ Misses        168      130      -38     

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

This is a great improvement, both to tests and implementation. Thanks a lot for the refactor, @jbandoro !

I left a minor comment related to naming.

tests/operators/test_base.py Show resolved Hide resolved
cosmos/operators/base.py Outdated Show resolved Hide resolved
Copy link

netlify bot commented Jan 5, 2024

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit 4997eda
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/659861f3c999ca000836dda3

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.

Great improvement, thanks for the refactor, @jbandoro !

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 8, 2024
@tatiana tatiana merged commit 380ac52 into astronomer:main Jan 8, 2024
42 checks passed
tatiana added a commit that referenced this pull request Jan 10, 2024
Bug fixes

* Fix disable event tracking throwing error by @jbandoro in #784
* Fix support for string path for LoadMode.DBT_LS_FILE and docs by @Flinz in #788
* Remove stack trace to disable unnecessary K8s error by @tatiana in #790

Others

* Update examples to use the astro-runtime 10.0.0 by @RNHTTR in #777
* Docs: add missing imports for mwaa getting started by @Benjamin0313 in #792
* Refactor common executor constructors with test coverage by @jbandoro in #774
* pre-commit updates in #789
@tatiana tatiana mentioned this pull request Jan 10, 2024
tatiana added a commit that referenced this pull request Jan 11, 2024
**Bug fixes**

* Fix disable event tracking throwing error by @jbandoro in #784
* Fix support for string path for `LoadMode.DBT_LS_FILE` and docs by
@Flinz in #788
* Remove stack trace to disable unnecessary K8s error by @tatiana in
#790

**Others**

* Update examples to use the astro-runtime 10.0.0 by @RNHTTR in #777
* Docs: add missing imports for mwaa getting started by @Benjamin0313 in
#792
* Refactor common executor constructors with test coverage by @jbandoro
in #774
* pre-commit updates in #789
tatiana pushed a commit that referenced this pull request Jan 24, 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
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)
arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this pull request Jul 14, 2024
)

I noticed in astronomer#771 that there was a lot of repeated class constructors in
order to add a new execution mode that is common among `local`, `docker`
and `kubernetes` and there is no test coverage for the constructors and
methods in some of the operators.

This PR attempts to make it easier to add new execution operators in the
future.

## Breaking Change?
None

There may be task UI color differences with the kuberentes/docker
operators, since now all of LS/Seed/Run etc. operators across execution
modes have the same task colors.
arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this pull request Jul 14, 2024
**Bug fixes**

* Fix disable event tracking throwing error by @jbandoro in astronomer#784
* Fix support for string path for `LoadMode.DBT_LS_FILE` and docs by
@Flinz in astronomer#788
* Remove stack trace to disable unnecessary K8s error by @tatiana in
astronomer#790

**Others**

* Update examples to use the astro-runtime 10.0.0 by @RNHTTR in astronomer#777
* Docs: add missing imports for mwaa getting started by @Benjamin0313 in
astronomer#792
* Refactor common executor constructors with test coverage by @jbandoro
in astronomer#774
* pre-commit updates in astronomer#789
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
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:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants