Skip to content

modify FacebookAdsReportingHook to pass Facebook access token in the … #25453

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.

Sign up for GitHub

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

Closed
adaezebestow wants to merge 2 commits into from
Closed

modify FacebookAdsReportingHook to pass Facebook access token in the … #25453

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77f113d
modify FacebookAdsReportingHook to pass Facebook access token in the …
adaezebestow Aug 1, 2022
4902234
Update airflow/providers/facebook/ads/hooks/ads.py
adaezebestow Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion airflow/providers/facebook/ads/hooks/ads.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ def facebook_ads_config(self) -> Dict:
"""
self.log.info("Fetching fb connection: %s", self.facebook_conn_id)
conn = self.get_connection(self.facebook_conn_id)
config = conn.extra_dejson
config = {**conn.extra_dejson, "access_token": conn.password}

@josh-fell josh-fell Aug 3, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should there be some warning/message to the user noting that they should set access_token in the Connection's password attr rather than in Extras as well?

Side note, this hook desperately needs some documentation re: setting up a connection.

@uranusjr uranusjr Aug 3, 2022

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.

Yeah, or (if we decide setting the token in extras is acceptable), there should be some checks to make sure the empty password does not override the token in extras.

@adaezebestow adaezebestow Aug 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's appropriate for the access_token to be passed in the extra parameter since it is essentially the password for the Facebook API and should not be exposed. +1 need for documentation.

missing_keys = self.client_required_fields - config.keys()
if missing_keys:
message = f"{missing_keys} fields are missing"
Expand Down