Added `overrides` parameter to CloudRunExecuteJobOperator by ChloeSheasby · Pull Request #34874 · apache/airflow · GitHub
Skip to content

Added overrides parameter to CloudRunExecuteJobOperator#34874

Merged
eladkal merged 6 commits into
apache:mainfrom
ChloeSheasby:add-overrides-cloud-run-job
Oct 25, 2023
Merged

Added overrides parameter to CloudRunExecuteJobOperator#34874
eladkal merged 6 commits into
apache:mainfrom
ChloeSheasby:add-overrides-cloud-run-job

Conversation

@ChloeSheasby

@ChloeSheasby ChloeSheasby commented Oct 11, 2023

Copy link
Copy Markdown
Contributor

This PR aims to add the functionality of the "overrides" variable for the CloudRunExecuteJobOperator as provided by the google.cloud.run.v2 package.

Notes:

  • The "overrides" variable requires specific authorization for it to be used.
  • This means that the tests around the RunJobRequest message are failing.
  • I believe that a service account managed by Apache/Airflow needs to be updated for this to be completed.

@boring-cyborg

boring-cyborg Bot commented Oct 11, 2023

Copy link
Copy Markdown

Taragolis
Taragolis reviewed Oct 11, 2023
View reviewed changes
Comment thread airflow/providers/google/cloud/hooks/cloud_run.py Outdated
@ChloeSheasby

ChloeSheasby commented Oct 12, 2023

Copy link
Copy Markdown
Contributor Author

Looks like google-cloud-run pypi (repo is called googleapis/python-run) doesn't even have the overrides variable available in their API yet. I made an issue for adding this functionality on the googleapis/python-run repo.
googleapis/python-run#135

@ChloeSheasby ChloeSheasby mentioned this pull request Oct 12, 2023
Closed
@ChloeSheasby

ChloeSheasby commented Oct 16, 2023

Copy link
Copy Markdown
Contributor Author

Functionality is there and will be released in v0.10.0: googleapis/python-run#127

@ChloeSheasby ChloeSheasby force-pushed the add-overrides-cloud-run-job branch from e56daad to 6e8f42a Compare October 20, 2023 16:10
@ChloeSheasby ChloeSheasby marked this pull request as ready for review October 20, 2023 16:11
cjames23
cjames23 reviewed Oct 20, 2023
View reviewed changes
Comment thread airflow/providers/google/cloud/operators/cloud_run.py Outdated

@cjames23 cjames23 Oct 20, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The type for overrides should indicate the type for the key and the values, though most often it is dict[str, Any] a dictionary allows other types for the key such as integers.

@ChloeSheasby ChloeSheasby Oct 20, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are you saying I need to make sure that the type of the dictionary is more specific?

@cjames23 cjames23 Oct 20, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes
Something like

overrides: dict[str, Any]  | None = None

If that is the expect key type.

@amoghrajesh amoghrajesh Oct 22, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What kind of dict are we expecting here? If it is defined well, I prefer adding typing to it. If not, do dict[str, Any]

@ChloeSheasby ChloeSheasby Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cjames23 @amoghrajesh I modified the dictionary type based on what is expected here: https://github.com/googleapis/python-run/blob/main/google/cloud/run_v2/types/job.py#L258

@Taragolis Taragolis Oct 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As I could see some of the values in this dictionary could be integers, so better use dict[str, Any] | None

@Taragolis

Taragolis commented Oct 24, 2023

Copy link
Copy Markdown
Contributor

Need to fix remaining Static Checks

For quick fix would be enough to install pre-commit and run ruff and black

pip install pre-commit

pre-commit install  # should be run in the cloned airflow repo directory
pre-commit run black --all-files
pre-commit run ruff --all-files

After that add files and push into repo

@ChloeSheasby ChloeSheasby force-pushed the add-overrides-cloud-run-job branch from 7021f48 to 85aca2f Compare October 25, 2023 14:11
Taragolis
Taragolis approved these changes Oct 25, 2023
View reviewed changes

@Taragolis Taragolis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

@ChloeSheasby ChloeSheasby force-pushed the add-overrides-cloud-run-job branch from 85aca2f to 11b4237 Compare October 25, 2023 18:18
cjames23
cjames23 approved these changes Oct 25, 2023
View reviewed changes

@cjames23 cjames23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

@eladkal eladkal changed the title Added "overrides" parameter for CloudRunExecuteJobOperator Added overrides parameter for CloudRunExecuteJobOperator Oct 25, 2023
@eladkal eladkal changed the title Added overrides parameter for CloudRunExecuteJobOperator Added overrides parameter to CloudRunExecuteJobOperator Oct 25, 2023
@eladkal eladkal merged commit 0bb5631 into apache:main Oct 25, 2023
@boring-cyborg

boring-cyborg Bot commented Oct 25, 2023

Copy link
Copy Markdown

@ChloeSheasby ChloeSheasby deleted the add-overrides-cloud-run-job branch October 25, 2023 19:22
@eladkal eladkal mentioned this pull request Oct 28, 2023
Closed
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Taragolis Taragolis Taragolis approved these changes
@eladkal eladkal eladkal approved these changes
@amoghrajesh amoghrajesh Awaiting requested review from amoghrajesh
+1 more reviewer
@cjames23 cjames23 cjames23 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:providers area:system-tests kind:documentation provider:google Google (including GCP) related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ChloeSheasby @Taragolis @cjames23 @amoghrajesh @eladkal