-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
[Part 2] Calendar to GCS Operator #20769
Conversation
I would like some help on how to do system tests. I have created the example dag and system test file; but I am not sure how to test it. I read through this guide. I think I need make some changes to variables.env file; but I am not sure what these changes should be. Also, I am not sure about the --forward-credentials option for running system tests. Which credentials will it forward? Do I need to put the credentials somewhere? |
From your example DAG it looks like you need to set up two variables:
Apart from that you will need to add a credentials key because you are using it in test: @pytest.mark.credential_file(GCP_GCS_KEY) The file should be named
Regarding this question:
You need to use credentials to some GCP project. As far as I know we don't have airflow project that can be used for this purpose (cc @potiuk to confirm). |
Thank you! |
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.
Looks pretty good! Just a few suggestions.
|
||
.. seealso:: | ||
For more information on how to use this operator, take a look at the guide: | ||
:ref:TODO `howto/operator:GoogleCalendarToGCSOperator` |
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.
Is this TODO
still needed? The associated doc looks relatively complete.
You can always mark PRs as a draft first if you still have some WIP actions you'd like to do before you'd like a more detailed review.
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.
Actually I marked it as TODO
because I think it needs the doc url.. which I assumed would be created after I merged this PR.
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.
Removed TODO
docs/apache-airflow-providers-google/operators/transfer/calendar_to_gcs.rst
Outdated
Show resolved
Hide resolved
docs/apache-airflow-providers-google/operators/transfer/calendar_to_gcs.rst
Outdated
Show resolved
Hide resolved
Thank you for all your help @turbaszek. Could you help take a look? This gives the error location: self = <subprocess.Popen object at 0x4065c8b250> Here is the error: I tried running |
fcdd63b
to
34babc8
Compare
60a9bc9
to
2b70f38
Compare
@turbaszek |
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.
LGTM
@rsg17 can you please rebase
If no further comments i'll merge after
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
1eeff52
to
175458b
Compare
Rebased. Thank you! |
related: #8471
This PR adds a Google Calendar to GCS Operator to write Google Calendar Events to GCS.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.