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

Apply PROVIDE_PROJECT_ID mypy workaround across Google provider #39129

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Apr 19, 2024

There is a simple workaround implemented several years ago for Google provider project_id default value being PROVIDE_PROJECT_ID that satisfy mypy checks for project_id being set. They way how fallback_to_default_project_id works is that across all the providers the project_id is actually set, even if technically it's default value is set to None.

This is similar typing workaround as we use for NEW_SESSION in the core of Airflow.

The workaround has not been applied consistently across all the google provider code and occasionally it causes MyPy complaining when newer version of a google library introduces more strict type checking and expects the provider_id to be set.

This PR applies the workaround across all the Google provider code.

This is - generally speaking a no-op operation. Nothing changes, except MyPy being aware that the project_id is actually going to be set even if it is technically set to None.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:logging area:providers provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues labels Apr 19, 2024
@potiuk
Copy link
Member Author

potiuk commented Apr 19, 2024

cc: @VladaZakharova

@potiuk
Copy link
Member Author

potiuk commented Apr 19, 2024

cc: @moiseenkov

@potiuk potiuk marked this pull request as draft April 19, 2024 11:20
@potiuk potiuk force-pushed the apply-project-id-workaround branch from ec62247 to e42f8aa Compare April 19, 2024 11:31
@potiuk potiuk marked this pull request as ready for review April 19, 2024 11:42
@potiuk potiuk added upgrade to newer dependencies If set, upgrade to newer dependencies is forced default versions only When assigned to PR - only default python version is used for CI tests full tests needed We need to run full set of tests for this PR to merge labels Apr 19, 2024
There is a simple workaround implemented several years ago for Google
provider `project_id` default value being PROVIDE_PROJECT_ID that
satisfy mypy checks for project_id being set. They way how
`fallback_to_default_project_id` works is that across all the
providers the project_id is actually set, even if technically
it's default value is set to None.

This is similar typing workaround as we use for NEW_SESSION in the
core of Airflow.

The workaround has not been applied consistently across all the
google provider code and occasionally it causes MyPy complaining
when newer version of a google library introduces more strict
type checking and expects the provider_id to be set.

This PR applies the workaround across all the Google provider
code.

This is - generally speaking a no-op operation. Nothing changes,
except MyPy being aware that the project_id is actually going to
be set even if it is technically set to None.
@potiuk potiuk force-pushed the apply-project-id-workaround branch from e42f8aa to 5d1b885 Compare April 19, 2024 11:47
@potiuk
Copy link
Member Author

potiuk commented Apr 19, 2024

Rerunning with upgrade + full tests needed just in case.

@potiuk
Copy link
Member Author

potiuk commented Apr 19, 2024

Random /flaky failure . Merging. All looks good. In the future (providing we keep the approach) it should not cause unexpected mypy issues.

@potiuk potiuk merged commit 90acbfb into apache:main Apr 19, 2024
64 of 65 checks passed
@potiuk potiuk deleted the apply-project-id-workaround branch April 19, 2024 12:22
potiuk added a commit that referenced this pull request Apr 19, 2024
There is a simple workaround implemented several years ago for Google
provider `project_id` default value being PROVIDE_PROJECT_ID that
satisfy mypy checks for project_id being set. They way how
`fallback_to_default_project_id` works is that across all the
providers the project_id is actually set, even if technically
it's default value is set to None.

This is similar typing workaround as we use for NEW_SESSION in the
core of Airflow.

The workaround has not been applied consistently across all the
google provider code and occasionally it causes MyPy complaining
when newer version of a google library introduces more strict
type checking and expects the provider_id to be set.

This PR applies the workaround across all the Google provider
code.

This is - generally speaking a no-op operation. Nothing changes,
except MyPy being aware that the project_id is actually going to
be set even if it is technically set to None.

(cherry picked from commit 90acbfb)
@potiuk potiuk added this to the Airflow 2.9.1 milestone Apr 19, 2024
@potiuk potiuk added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 19, 2024
@potiuk
Copy link
Member Author

potiuk commented Apr 19, 2024

cc: @ephraimbuddy @jedcunningham -> I cherry-picked that one to v2-9-test - there are some imports from airflow tp google that mypy follows and reports as issues - but this one (purely provider's change) - should prevent future similar issues.

utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
…he#39129)

There is a simple workaround implemented several years ago for Google
provider `project_id` default value being PROVIDE_PROJECT_ID that
satisfy mypy checks for project_id being set. They way how
`fallback_to_default_project_id` works is that across all the
providers the project_id is actually set, even if technically
it's default value is set to None.

This is similar typing workaround as we use for NEW_SESSION in the
core of Airflow.

The workaround has not been applied consistently across all the
google provider code and occasionally it causes MyPy complaining
when newer version of a google library introduces more strict
type checking and expects the provider_id to be set.

This PR applies the workaround across all the Google provider
code.

This is - generally speaking a no-op operation. Nothing changes,
except MyPy being aware that the project_id is actually going to
be set even if it is technically set to None.
RodrigoGanancia pushed a commit to RodrigoGanancia/airflow that referenced this pull request May 10, 2024
…he#39129)

There is a simple workaround implemented several years ago for Google
provider `project_id` default value being PROVIDE_PROJECT_ID that
satisfy mypy checks for project_id being set. They way how
`fallback_to_default_project_id` works is that across all the
providers the project_id is actually set, even if technically
it's default value is set to None.

This is similar typing workaround as we use for NEW_SESSION in the
core of Airflow.

The workaround has not been applied consistently across all the
google provider code and occasionally it causes MyPy complaining
when newer version of a google library introduces more strict
type checking and expects the provider_id to be set.

This PR applies the workaround across all the Google provider
code.

This is - generally speaking a no-op operation. Nothing changes,
except MyPy being aware that the project_id is actually going to
be set even if it is technically set to None.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logging area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) default versions only When assigned to PR - only default python version is used for CI tests full tests needed We need to run full set of tests for this PR to merge provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues upgrade to newer dependencies If set, upgrade to newer dependencies is forced
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants