Fix global flags for lists by ms32035 · Pull Request #863 · astronomer/astronomer-cosmos

ms32035 · 2024-02-27T01:29:23Z

Description

Correctly deals with global flags when they are a list.
Note - in the module there's no distinguishing which are and which aren't.

Related Issue(s)

Breaking Change?

Fixes the resulting bug in #838, though the type discrepancy is still there.

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

netlify · 2024-02-27T01:29:37Z

✅ Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name	Link
🔨 Latest commit	`4826d7b`
🔍 Latest deploy log	https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/662b9e6381dfe6000840e36a

tatiana

Thanks for working to fix this, @ms32035 !

Could you add a test that represents the original issue you found?

Would it make sense to fix the types as part of this PR? As you noticed, select and exclude are likely lists.

codecov · 2024-02-27T13:29:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.54%. Comparing base (73a9ba2) to head (4826d7b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #863      +/-   ##
==========================================
- Coverage   95.54%   95.54%   -0.01%     
==========================================
  Files          57       57              
  Lines        2763     2762       -1     
==========================================
- Hits         2640     2639       -1     
  Misses        123      123

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

ms32035 · 2024-02-27T14:06:05Z

I can add a test, but fixing the whole issue is beyond the scope of the PR. The problem is
you have a duplication of handling select and exclude flags
between

astronomer-cosmos/cosmos/operators/base.py

Line 66 in da50758

global_flags = (

and
https://github.com/astronomer/astronomer-cosmos/blob/main/cosmos/operators/base.py#L341
which you need to address on a design level

ms32035 · 2024-02-27T14:12:21Z

@tatiana maybe even this whole method is not needed, as it's handled by the global flags?
https://github.com/astronomer/astronomer-cosmos/blob/main/cosmos/operators/base.py#L341

tatiana · 2024-02-27T16:06:21Z

@tatiana maybe even this whole method is not needed, as it's handled by the global flags? https://github.com/astronomer/astronomer-cosmos/blob/main/cosmos/operators/base.py#L341

I think you're right, @ms32035 , could you delete this method and we can see if tests continue passing?

ms32035 · 2024-02-28T01:05:06Z

@tatiana this should now be addressed in my latest commit

ms32035 · 2024-03-03T22:23:44Z

@tatiana this should be good for merge now

tatiana

Thanks for working on this, @ms32035 ! We'll release it as part of 1.4

@dwreeves

Features * Add dbt docs natively in Airflow via plugin by @dwreeves in #737 * Add support for ``InvocationMode.DBT_RUNNER`` for local execution mode by @jbandoro in #850 * Support partial parsing to render DAGs faster when using ``ExecutionMode.LOCAL``, ``ExecutionMode.VIRTUALENV`` and ``LoadMode.DBT_LS`` by @dwreeves in #800 * Improve performance by 22-35% or more by caching partial parse artefact by @tatiana in #904 * Add Azure Container Instance as Execution Mode by @danielvdende in #771 * Add dbt build operators by @dylanharper-qz in #795 * Add dbt profile config variables to mapped profile by @ykuc in #794 * Add more template fields to ``DbtBaseOperator`` by @dwreeves in #786 * Add ``pip_install_options`` argument to operators by @octiva in #808 Bug fixes * Make ``PostgresUserPasswordProfileMapping`` schema argument optional by @FouziaTariq in #683 * Fix ``folder_dir`` not showing on logs for ``DbtDocsS3LocalOperator`` by @PrimOox in #856 * Improve ``dbt ls`` parsing resilience to missing tags/config by @tatiana in #859 * Fix ``operator_args`` modified in place in Airflow converter by @jbandoro in #835 * Fix Docker and Kubernetes operators execute method resolution by @jbandoro in #849 * Fix ``TrinoBaseProfileMapping`` required parameter for non method authentication by @AlexandrKhabarov in #921 * Fix global flags for lists by @ms32035 in #863 * Fix ``GoogleCloudServiceAccountDictProfileMapping`` when getting values from the Airflow connection ``extra__`` keys by @glebkrapivin in #923 * Fix using the dag as a keyword argument as ``specific_args_keys`` in DbtTaskGroup by @tboutaour in #916 * Fix ACI integration (``DbtAzureContainerInstanceBaseOperator``) by @danielvdende in #872 * Fix setting dbt project dir to the tmp dir by @dwreeves in #873 * Fix dbt docs operator to not use ``graph.gpickle`` file when ``--no-write-json`` is passed by @dwreeves in #883 * Make Pydantic a required dependency by @pankajkoti in #939 * Gracefully error if users try to ``emit_datasets`` with ``Airflow 2.9.0`` or ``2.9.1`` by @tatiana in #948 * Fix parsing tests that have no parents in #933 by @jlaneve * Correct ``root_path`` in partial parse cache by @pankajkoti in #950 Docs * Fix docs homepage link by @jlaneve in #860 * Fix docs ``ExecutionConfig.dbt_project_path`` by @jbandoro in #847 * Fix typo in MWAA getting started guide by @jlaneve in #846 * Fix typo related to exporting docs to GCS by @tboutaour in #922 * Improve partial parsing docs by @tatiana in #898 * Improve docs for datasets for airflow >= 2.4 by @SiddiqueAhmad in #879 * Improve test behaviour docs to highlight ``warning`` feature in the ``virtualenv`` mode by @mc51 in #910 * Fix docs typo by @SiddiqueAhmad in #917 * Improve Astro docs by @RNHTTR in #951 Others * Add performance integration tests by @jlaneve in #827 * Enable ``append_env`` in ``operator_args`` by default by @tatiana in #899 * Change default ``append_env`` behaviour depending on Cosmos ``ExecutionMode`` by @pankajkoti and @pankajastro in #954 * Expose the ``dbt`` graph in the ``DbtToAirflowConverter`` class by @tommyjxl in #886 * Improve dbt docs plugin rendering padding by @dwreeves in #876 * Add ``connect_retries`` to databricks profile to fix expensive integration failures by @jbandoro in #826 * Add import sorting (isort) to Cosmos by @jbandoro in #866 * Add Python 3.11 to CI/tests by @tatiana and @jbandoro in #821, #824 and #825 * Fix failing ``test_created_pod`` for ``apache-airflow-providers-cncf-kubernetes`` after v8.0.0 update by @jbandoro in #854 * Extend ``DatabricksTokenProfileMapping`` test to include session properties by @tatiana in #858 * Fix broken integration test uncovered from Pytest 8.0 update by @jbandoro in #845 * Add Apache Airflow 2.9 to the test matrix by @tatiana in #940 * Replace deprecated ``DummyOperator`` by ``EmptyOperator`` if Airflow >=2.4.0 by @tatiana in #900 * Improve logs to troubleshoot issue in 1.4.0a2 with astro-cli by @tatiana in #947 * Fix issue when publishing a new release to PyPI by @tatiana in #946 * Pre-commit hook updates in #820, #834, #843 and #852, #890, #896, #901, #905, #908, #919, #931, #941

Correctly deals with global flags when they are a list. Note - in the module there's no distinguishing which are and which aren't. Co-authored-by: Tatiana Al-Chueyr <tatiana.alchueyr@gmail.com>

@dwreeves

Features * Add dbt docs natively in Airflow via plugin by @dwreeves in astronomer#737 * Add support for ``InvocationMode.DBT_RUNNER`` for local execution mode by @jbandoro in astronomer#850 * Support partial parsing to render DAGs faster when using ``ExecutionMode.LOCAL``, ``ExecutionMode.VIRTUALENV`` and ``LoadMode.DBT_LS`` by @dwreeves in astronomer#800 * Improve performance by 22-35% or more by caching partial parse artefact by @tatiana in astronomer#904 * Add Azure Container Instance as Execution Mode by @danielvdende in astronomer#771 * Add dbt build operators by @dylanharper-qz in astronomer#795 * Add dbt profile config variables to mapped profile by @ykuc in astronomer#794 * Add more template fields to ``DbtBaseOperator`` by @dwreeves in astronomer#786 * Add ``pip_install_options`` argument to operators by @octiva in astronomer#808 Bug fixes * Make ``PostgresUserPasswordProfileMapping`` schema argument optional by @FouziaTariq in astronomer#683 * Fix ``folder_dir`` not showing on logs for ``DbtDocsS3LocalOperator`` by @PrimOox in astronomer#856 * Improve ``dbt ls`` parsing resilience to missing tags/config by @tatiana in astronomer#859 * Fix ``operator_args`` modified in place in Airflow converter by @jbandoro in astronomer#835 * Fix Docker and Kubernetes operators execute method resolution by @jbandoro in astronomer#849 * Fix ``TrinoBaseProfileMapping`` required parameter for non method authentication by @AlexandrKhabarov in astronomer#921 * Fix global flags for lists by @ms32035 in astronomer#863 * Fix ``GoogleCloudServiceAccountDictProfileMapping`` when getting values from the Airflow connection ``extra__`` keys by @glebkrapivin in astronomer#923 * Fix using the dag as a keyword argument as ``specific_args_keys`` in DbtTaskGroup by @tboutaour in astronomer#916 * Fix ACI integration (``DbtAzureContainerInstanceBaseOperator``) by @danielvdende in astronomer#872 * Fix setting dbt project dir to the tmp dir by @dwreeves in astronomer#873 * Fix dbt docs operator to not use ``graph.gpickle`` file when ``--no-write-json`` is passed by @dwreeves in astronomer#883 * Make Pydantic a required dependency by @pankajkoti in astronomer#939 * Gracefully error if users try to ``emit_datasets`` with ``Airflow 2.9.0`` or ``2.9.1`` by @tatiana in astronomer#948 * Fix parsing tests that have no parents in astronomer#933 by @jlaneve * Correct ``root_path`` in partial parse cache by @pankajkoti in astronomer#950 Docs * Fix docs homepage link by @jlaneve in astronomer#860 * Fix docs ``ExecutionConfig.dbt_project_path`` by @jbandoro in astronomer#847 * Fix typo in MWAA getting started guide by @jlaneve in astronomer#846 * Fix typo related to exporting docs to GCS by @tboutaour in astronomer#922 * Improve partial parsing docs by @tatiana in astronomer#898 * Improve docs for datasets for airflow >= 2.4 by @SiddiqueAhmad in astronomer#879 * Improve test behaviour docs to highlight ``warning`` feature in the ``virtualenv`` mode by @mc51 in astronomer#910 * Fix docs typo by @SiddiqueAhmad in astronomer#917 * Improve Astro docs by @RNHTTR in astronomer#951 Others * Add performance integration tests by @jlaneve in astronomer#827 * Enable ``append_env`` in ``operator_args`` by default by @tatiana in astronomer#899 * Change default ``append_env`` behaviour depending on Cosmos ``ExecutionMode`` by @pankajkoti and @pankajastro in astronomer#954 * Expose the ``dbt`` graph in the ``DbtToAirflowConverter`` class by @tommyjxl in astronomer#886 * Improve dbt docs plugin rendering padding by @dwreeves in astronomer#876 * Add ``connect_retries`` to databricks profile to fix expensive integration failures by @jbandoro in astronomer#826 * Add import sorting (isort) to Cosmos by @jbandoro in astronomer#866 * Add Python 3.11 to CI/tests by @tatiana and @jbandoro in astronomer#821, astronomer#824 and astronomer#825 * Fix failing ``test_created_pod`` for ``apache-airflow-providers-cncf-kubernetes`` after v8.0.0 update by @jbandoro in astronomer#854 * Extend ``DatabricksTokenProfileMapping`` test to include session properties by @tatiana in astronomer#858 * Fix broken integration test uncovered from Pytest 8.0 update by @jbandoro in astronomer#845 * Add Apache Airflow 2.9 to the test matrix by @tatiana in astronomer#940 * Replace deprecated ``DummyOperator`` by ``EmptyOperator`` if Airflow >=2.4.0 by @tatiana in astronomer#900 * Improve logs to troubleshoot issue in 1.4.0a2 with astro-cli by @tatiana in astronomer#947 * Fix issue when publishing a new release to PyPI by @tatiana in astronomer#946 * Pre-commit hook updates in astronomer#820, astronomer#834, astronomer#843 and astronomer#852, astronomer#890, astronomer#896, astronomer#901, astronomer#905, astronomer#908, astronomer#919, astronomer#931, astronomer#941

Fix global flags for lists

da50758

ms32035 requested a review from a team as a code owner February 27, 2024 01:29

dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Feb 27, 2024

ms32035 temporarily deployed to external February 27, 2024 01:29 — with GitHub Actions Inactive

dosubot Bot added the area:config Related to configuration, like YAML files, environment variables, or executer configuration label Feb 27, 2024

tatiana reviewed Feb 27, 2024

View reviewed changes

tatiana mentioned this pull request Feb 27, 2024

Release 1.4.0a1 #864

Merged

Fix tests for flags

568d74f

dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Feb 28, 2024

ms32035 had a problem deploying to external February 28, 2024 00:58 — with GitHub Actions Error

Merge branch 'main' into feature/global_flag_list

f7de8ec

ms32035 temporarily deployed to external February 28, 2024 01:03 — with GitHub Actions Inactive

tatiana reviewed Feb 29, 2024

View reviewed changes

Comment thread cosmos/operators/base.py Outdated

tatiana reviewed Feb 29, 2024

View reviewed changes

Comment thread tests/operators/test_local.py

fix multiple values case

dffcea9

ms32035 temporarily deployed to external March 1, 2024 09:13 — with GitHub Actions Inactive

Merge branch 'main' into feature/global_flag_list

ec69143

ms32035 temporarily deployed to external March 3, 2024 22:23 — with GitHub Actions Inactive

ms32035 had a problem deploying to external March 8, 2024 10:41 — with GitHub Actions Error

fix for empty list

2e6235b

ms32035 force-pushed the feature/global_flag_list branch from 7fe0ee7 to 2e6235b Compare March 8, 2024 10:57

ms32035 had a problem deploying to external March 8, 2024 10:57 — with GitHub Actions Error

reduce complexity

c76529d

ms32035 temporarily deployed to external March 11, 2024 08:54 — with GitHub Actions Inactive

Merge branch 'main' into feature/global_flag_list

42a5ca4

ms32035 had a problem deploying to external April 5, 2024 13:45 — with GitHub Actions Error

fix tests

a2aa061

ms32035 had a problem deploying to external April 5, 2024 14:53 — with GitHub Actions Error

Merge branch 'main' into feature/global_flag_list

b1ea3b7

tatiana temporarily deployed to external April 22, 2024 10:34 — with GitHub Actions Inactive

Merge branch 'main' into feature/global_flag_list

1ba39d2

ms32035 had a problem deploying to external April 26, 2024 11:52 — with GitHub Actions Error

improve test coverage

4826d7b

ms32035 temporarily deployed to external April 26, 2024 12:30 — with GitHub Actions Inactive

tatiana added this to the 1.4.0 milestone Apr 26, 2024

tatiana approved these changes Apr 26, 2024

View reviewed changes

dosubot Bot added the lgtm This PR has been approved by a maintainer label Apr 26, 2024

tatiana merged commit d1dbc41 into astronomer:main Apr 26, 2024

ms32035 deleted the feature/global_flag_list branch April 26, 2024 19:47

tatiana mentioned this pull request May 2, 2024

Release 1.4.0 #934

Merged

pankajkoti mentioned this pull request Jun 24, 2025

Invalid selector values passed to DbtTestMixin #838

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix global flags for lists#863

Fix global flags for lists#863
tatiana merged 12 commits into
astronomer:mainfrom
ms32035:feature/global_flag_list

ms32035 commented Feb 27, 2024 •

edited

Loading

Uh oh!

netlify Bot commented Feb 27, 2024 •

edited

Loading

Uh oh!

tatiana left a comment

Uh oh!

codecov Bot commented Feb 27, 2024 •

edited

Loading

Uh oh!

ms32035 commented Feb 27, 2024

Uh oh!

ms32035 commented Feb 27, 2024

Uh oh!

tatiana commented Feb 27, 2024

Uh oh!

ms32035 commented Feb 28, 2024

Uh oh!

Uh oh!

Uh oh!

ms32035 commented Mar 3, 2024

Uh oh!

tatiana left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ms32035 commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Breaking Change?

Checklist

Uh oh!

netlify Bot commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for sunny-pastelito-5ecb04 canceled.

Uh oh!

tatiana left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ms32035 commented Feb 27, 2024

Uh oh!

ms32035 commented Feb 27, 2024

Uh oh!

tatiana commented Feb 27, 2024

Uh oh!

ms32035 commented Feb 28, 2024

Uh oh!

Uh oh!

Uh oh!

ms32035 commented Mar 3, 2024

Uh oh!

tatiana left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ms32035 commented Feb 27, 2024 •

edited

Loading

netlify Bot commented Feb 27, 2024 •

edited

Loading

codecov Bot commented Feb 27, 2024 •

edited

Loading