Skip to content
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

Custom Facebook Ads Operator #8008

Merged
merged 1 commit into from
Apr 14, 2020
Merged

Conversation

randr97
Copy link
Contributor

@randr97 randr97 commented Mar 30, 2020

Closes #7887


Issue link: WILL BE INSERTED BY boring-cyborg

Make sure to mark the boxes below before creating PR: [x]


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.
Read the Pull Request Guidelines for more information.

@randr97 randr97 force-pushed the FacebookOperator branch 2 times, most recently from 765b87e to 43abe6b Compare March 30, 2020 09:33
@mik-laj mik-laj self-requested a review March 30, 2020 09:36
@randr97 randr97 changed the title Custom Facebook Ads Operator #7887 Custom Facebook Ads Operator Mar 30, 2020
@randr97 randr97 force-pushed the FacebookOperator branch 7 times, most recently from 542db01 to 5040038 Compare March 31, 2020 10:54
@randr97 randr97 force-pushed the FacebookOperator branch 4 times, most recently from 0947c6f to ed5efc0 Compare March 31, 2020 19:28
@codecov-io
Copy link

codecov-io commented Mar 31, 2020

Codecov Report

Merging #8008 into master will decrease coverage by 0.44%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8008      +/-   ##
==========================================
- Coverage   88.45%   88.00%   -0.45%     
==========================================
  Files         937      940       +3     
  Lines       45234    45353     +119     
==========================================
- Hits        40011    39913      -98     
- Misses       5223     5440     +217     
Impacted Files Coverage Δ
airflow/models/connection.py 95.07% <ø> (ø)
airflow/providers/facebook/ads/hooks/ads.py 87.93% <87.93%> (ø)
...le/facebook_ads_to_gcs/example_dags/example_ads.py 100.00% <100.00%> (ø)
...viders/google/facebook_ads_to_gcs/operators/ads.py 100.00% <100.00%> (ø)
airflow/utils/db.py 98.37% <100.00%> (+0.01%) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0.00%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0.00%> (-47.06%) ⬇️
airflow/security/kerberos.py 30.43% <0.00%> (-45.66%) ⬇️
airflow/kubernetes/pod_launcher.py 47.18% <0.00%> (-45.08%) ⬇️
airflow/executors/sequential_executor.py 56.00% <0.00%> (-44.00%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45c8983...ff2fcf6. Read the comment docs.

@randr97 randr97 force-pushed the FacebookOperator branch 4 times, most recently from 9460b5d to 5234efa Compare April 8, 2020 05:13
@randr97 randr97 requested a review from mik-laj April 8, 2020 05:16
@potiuk
Copy link
Member

potiuk commented Apr 13, 2020

Hey @randr97 -> I'd love to merge this one. Can you please rebase and regenerate the requirements?

@randr97
Copy link
Contributor Author

randr97 commented Apr 13, 2020

Hey @randr97 -> I'd love to merge this one. Can you please rebase and regenerate the requirements?

Sure thing, will do

@potiuk
Copy link
Member

potiuk commented Apr 13, 2020

I think there was a change in MyPy in the meantime and you will have to wait until #8267 is merged (it fixes the problem) and rebase again I am afraid. Sorry for that.

@potiuk
Copy link
Member

potiuk commented Apr 13, 2020

OK. You can rebase now - it should be fine :)

@randr97 randr97 force-pushed the FacebookOperator branch 3 times, most recently from ea49be3 to f0fac92 Compare April 13, 2020 19:12
Comment on lines +248 to +256
{
"facebook_ads_client": {
"account_id": "act_123456789",
"app_id": "1234567890",
"app_secret": "1f45tghxxxx12345",
"access_token": "ABcdEfghiJKlmnoxxyz"
}
}
""",
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if we should keep dummy values here like that. Can we do this likewise gcp? schema="default"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any better way to show the schema ?? Cus for GCP there is a tutorial on google's website. Integrating with airflow. Any thoughts?

@randr97 randr97 force-pushed the FacebookOperator branch 3 times, most recently from 5320f76 to 7217064 Compare April 13, 2020 21:41
@potiuk potiuk merged commit eee4eba into apache:master Apr 14, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Apr 14, 2020

Awesome work, congrats on your first merged pull request!

@potiuk
Copy link
Member

potiuk commented Apr 14, 2020

Thanks @randr97 !

@randr97 randr97 deleted the FacebookOperator branch April 15, 2020 20:21
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.

Custom Facebook Ads Operator
5 participants