-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Add CloudRunExecuteJobOperator #28525
Add CloudRunExecuteJobOperator #28525
Conversation
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)
|
2c8fce5
to
20bfa7b
Compare
20bfa7b
to
48eed2e
Compare
def get_conn(self) -> build: | ||
"""Returns a Google Cloud Run service object.""" | ||
http_authorized = self._authorize() | ||
|
||
# Use a regional endpoint since job execution is not possible from the global endpoint | ||
client_options = ClientOptions(api_endpoint=f'https://{self.region}-run.googleapis.com') | ||
return build('run', 'v1', client_options=client_options, | ||
http=http_authorized, cache_discovery=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we are not using JobsClient
from python SDK? https://cloud.google.com/python/docs/reference/run/latest/google.cloud.run_v2.services.jobs.JobsClient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, the v2 API and its Python client libraries should be much easier to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Taragolis would you have an idea of how to fix this?
If using the official Python Client and implementing all the CRUD operations on jobs are completion requirements for this PR, we should mark this PR as draft for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI Completion requirements were discussed in this issue : #24730.
Maybe we could consider all the Jobs CRUD operations in a later PR, when the official JobsClient can be installed properly?
Opened to suggestions here 😉
104d1ef
to
420c506
Compare
420c506
to
fabda1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve in advance (need to resolve doc spellcheck)
I'm not familiar with GCP, for me looks nice. So would be nice if someone else review it
fabda1d
to
2a95a05
Compare
2a95a05
to
d2a90cc
Compare
ab3e06e
to
c58f4a0
Compare
tests are failing :( |
I'm assuming that by merging this PR we won't need #27638 ? |
CloudRun is a container hosting service and CloudRunJobs are a job-based container execution version of that - IMO, we would need both sets of operators in Airflow to support both functionalities |
I'll try to fix tests in the coming days. |
7265c97
to
334efe5
Compare
Tests should be fixed now @eladkal . |
approved |
Looks like the failing test is not related to my implementation. I believe I should wait for a fix and rebase my branch from time to time? |
334efe5
to
7f19415
Compare
Checks seem ok (appart from 9 skipped) @eladkal 🙂 |
waiting for your comment on #28525 (comment) |
@VinceLegendre We bumped many google packages #30067 |
- New hook : CloudRunJobHook - New links : CloudRunJobLink and CloudRunJobExecutionLinks - New operator : CloudRunExecuteJobOperator - According tests - Documentation and examples
7f19415
to
587504e
Compare
I just rebased it to see if it works after recent upgrade of deps. I don't think there will be a big problem, It does not use any of the google client specific libraries - but the discovery client, so it should be fine as-is. |
However. I think indeed. it would be better to use the actual cloud run client rather than discovery one now. It should be possible now, after all the dependencies are updated, So the reason why discovery client was used here is gone. @VinceLegendre ? |
No worries. It can wait |
Hi @VinceLegendre checking if you still plan to make the changes? |
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. |
This operator aims at executing existing Cloud Run jobs, with the same design philosophy as the
DataflowStartFlexTemplateOperator
.CloudRunExecuteJobOperator
extends theBaseOperator
CloudRunJobHook
extends theGoogleBaseHook
, to easily manage authentication and project_id fallbackCloudRunJobLink
andCloudRunJobExecutionLink
extend theBaseGoogleLink
Closes: #24730