You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/08/01 22:23:07 UTC
[GitHub] [airflow] adaezebestow opened a new pull request, #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
adaezebestow opened a new pull request, #25453:
URL: https://github.com/apache/airflow/pull/25453
…password parameter so it's not exposed in the Astronomer UI
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25453:
URL: https://github.com/apache/airflow/pull/25453#issuecomment-1203183374
It needs fixing the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] adaezebestow commented on pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
Posted by GitBox <gi...@apache.org>.
adaezebestow commented on PR #25453:
URL: https://github.com/apache/airflow/pull/25453#issuecomment-1205448238
> It needs fixing the tests.
Still new to adding PRs to open source. I'd appreciate some guidance on how to resolve test failures and how to make this contribution more robust.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #25453:
URL: https://github.com/apache/airflow/pull/25453#issuecomment-1205501042
For the deprecation/warning piece, you can take a look at [this EKS hook](https://github.com/apache/airflow/blob/2bf3dc6314de8d14e05829492d34ae8d4f68263c/airflow/providers/amazon/aws/hooks/eks.py#L512-L517). It's a typical example of checking if a value/parameter is used and alerting the user that it's been deprecated. Hopefully this helps 🙂.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] closed pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
URL: https://github.com/apache/airflow/pull/25453
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25453:
URL: https://github.com/apache/airflow/pull/25453#discussion_r935126336
##########
airflow/providers/facebook/ads/hooks/ads.py:
##########
@@ -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}}
Review Comment:
```suggestion
config = {**conn.extra_dejson, "access_token": conn.password}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] adaezebestow commented on a diff in pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
Posted by GitBox <gi...@apache.org>.
adaezebestow commented on code in PR #25453:
URL: https://github.com/apache/airflow/pull/25453#discussion_r937975117
##########
airflow/providers/facebook/ads/hooks/ads.py:
##########
@@ -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}
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25453:
URL: https://github.com/apache/airflow/pull/25453#issuecomment-1205459334
> > It needs fixing the tests.
>
> Still new to adding PRs to open source. I'd appreciate some guidance on how to resolve test failures and how to make this contribution more robust.
Just look at the failing thests (the red stufl in checks) - and you need to simply run and fix those Pytest tests that failed. Generally in your case when you have good development environment set (Following https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst) and some IDE (PyCharm/VSCode) running and reproducing the failing tests should be easy. You can also run tests in Breeze environment (makes it easy to reproduce the CI failures but then integration with IDE is a bit more complex). All about running test, which kind of tests etc. is here: https://github.com/apache/airflow/blob/main/TESTING.rst - you even have screenshots from the IDEs etc.
There are a lot of materials, but it is all designed as self-learning exercise that you can do in your time. There is no hurry. Just learning on your own how to do it is a great way to improve your skills.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #25453:
URL: https://github.com/apache/airflow/pull/25453#issuecomment-1250419392
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25453:
URL: https://github.com/apache/airflow/pull/25453#discussion_r936258159
##########
airflow/providers/facebook/ads/hooks/ads.py:
##########
@@ -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}
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a diff in pull request #25453: modify FacebookAdsReportingHook to pass Facebook access token in the …
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #25453:
URL: https://github.com/apache/airflow/pull/25453#discussion_r936183279
##########
airflow/providers/facebook/ads/hooks/ads.py:
##########
@@ -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}
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org