Skip to content

Conversation

@ChloeSheasby
Copy link
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
Copy link

boring-cyborg bot commented Oct 11, 2023

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@ChloeSheasby
Copy link
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
Copy link
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
Copy link
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.

Copy link
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?

Copy link
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.

Copy link
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]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
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
Copy link
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

Copy link
Contributor

@Taragolis Taragolis left a comment

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
Copy link
Contributor

@cjames23 cjames23 left a comment

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
Copy link

boring-cyborg bot commented Oct 25, 2023

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants