Skip to content

Add Cloud Run Operator - #27638

Closed
mattsax wants to merge 4 commits into
apache:mainfrom
mattsax:add_cloud_run_operator
Closed

Add Cloud Run Operator#27638
mattsax wants to merge 4 commits into
apache:mainfrom
mattsax:add_cloud_run_operator

Conversation

@mattsax

@mattsax mattsax commented Nov 12, 2022

Copy link
Copy Markdown

This PR add a CloudRunOperator along with CloudRunHook.
This new Operator allows to make authenticated call against Cloud Run (HTTP request with proper authentication header)

CloudRunOperator extends the SimpleHttpOperator: this add a dependency to HTTP provider.
The authentication token needed in HTTP header (for proper Cloud Run authentication) is an ID Token.
The credential_provider.py had to be modified with a new function get_id_token_credentials to provide "ID Token like" credentials.


^ 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 commented Nov 12, 2022

Copy link
Copy Markdown

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 (flake8, 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.
    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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The two seems indeed related. But id_token_credentials.py provide the credentials from Google application default credentials.
Whereas the credential_provider.py already contains all the code to infer credentials from an Airflow Connection

@mattsax
mattsax force-pushed the add_cloud_run_operator branch from 0fd129f to 3df24fc Compare November 16, 2022 09:43
@potiuk
potiuk force-pushed the add_cloud_run_operator branch from 3df24fc to fa49d44 Compare November 17, 2022 18:02
@potiuk

potiuk commented Nov 17, 2022

Copy link
Copy Markdown
Member

Static checks failing.

@mattsax

mattsax commented Nov 18, 2022

Copy link
Copy Markdown
Author

I fixed the doc Build.
But for the static checks, the failure comes from airflow/providers/neo4j/hooks/neo4j.py. It doesn't seem related to this PR.

@potiuk

potiuk commented Nov 25, 2022

Copy link
Copy Markdown
Member

Conflicts need to be solved.

@mattsax
mattsax force-pushed the add_cloud_run_operator branch from f3dcd22 to 45ce866 Compare November 27, 2022 15:08
@potiuk

potiuk commented Dec 5, 2022

Copy link
Copy Markdown
Member

And errors to fix.

@eladkal

eladkal commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

I rebased the PR. lets see if test passes

@steren

steren commented Feb 6, 2023

Copy link
Copy Markdown

Hi, I am the Product Manager for Cloud Run. Thanks for working on a Cloud Run Operator for Airflow!

Please feel free to ask any Cloud Run related question here.

I looked at the PR, it seems that it is using an HTTP call to a Cloud Run service. I would have believed that a Cloud Run job would be more suited for Airflow, as it doesn't require the container to expose any HTTP server, it can be executed via a simple API call. Have you evaluated a job?

@matthieuvilatte

Copy link
Copy Markdown

Hi @steren
Depending on the use case we use both Cloud Run Service and Cloud Run Jobs.
(An other PR has been submited to add a Cloud Run Job operator )
I deployed dbt using Cloud Run. It is pretty convient to be abble to send parameters though a POST HTTP request: start_date and end_date or even --full-refresh parameter when building an incremental model.

@steren

steren commented Feb 8, 2023

Copy link
Copy Markdown

@matthieuvilatte Ah I might have misunderstood this Operator. It doesn't deploy a Cloud Run service but calls an existing Cloud Run service, is that right?

Maybe the Operator name should make this clearer (i.e. Cloud Run hook)? To differentiate it with the other "Cloud Run Job" Operator

@matthieuvilatte

Copy link
Copy Markdown

@steren
This Operator doesn't deploy a new Cloud Run Service, but can be used to make an authenticated call to an exising Cloud Run Service using a Service Account stored in Airflow Connections.

This Operator name is indeed confusing, but I don't think we should rename it Cloud Run hook. I might be wrong but when describing Airflow DAGs, we always use Operators. An Operator holds the execute method which is called when executing the task and uses a hook to deal with the connection.

May be Cloud Run Service Call Operator?

@eladkal

eladkal commented Feb 23, 2023

Copy link
Copy Markdown
Contributor

@matthieuvilatte operators usually design to do a specific action thus we like/prefer when operator has a verb in it's name indicate the action for example: SqlExecuteOperator, EC2StartInstanceOperator, WasbDeleteBlobOperator

@eladkal

eladkal commented Feb 24, 2023

Copy link
Copy Markdown
Contributor

Tests are failing.
Could you please fix it?

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Apr 10, 2023
@github-actions github-actions Bot closed this Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers area:system-tests kind:documentation provider:google Google (including GCP) related issues stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants