You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "TJaniF (via GitHub)" <gi...@apache.org> on 2023/03/01 16:13:59 UTC
[GitHub] [airflow] TJaniF opened a new pull request, #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
TJaniF opened a new pull request, #29842:
URL: https://github.com/apache/airflow/pull/29842
When using a templated `tag_name` in the current GithubTagSensor, the templated argument will not correctly be passed to the call to the GithubSensor.poke method to be used in the `tag_checker` method that is passed in as the `result_processor` function.
The result is that the sensor is looking for a tag of the format of the string template as shown in the logs:
```text
[2023-03-01, 15:44:20 UTC] {custom_githubtagsensor.py:191} INFO - Poking for tag: sync-metadata/1 in repository: TJaniF/airflow-fivetran-tutorial
[2023-03-01, 15:44:20 UTC] {base.py:73} INFO - Using connection ID 'github_conn' for task execution.
[2023-03-01, 15:44:21 UTC] {custom_githubtagsensor.py:210} INFO - Tag {{ ti.xcom_pull(task_ids='generate_tag_to_await', key='return_value') }} doesn't exists in TJaniF/airflow-fivetran-tutorial repository yet.
```
This PR adds an `allow_templates_in_result_processor` parameter to the GithubSensor, False by default to allow for backwards compatibility for users who have built custom operators on top of the GithubSensor. If `allow_templates_in_result_processor` is set to True as the GithubTagSensor now does by default, a dict containing the values of all templated arguments can be passed to the call to `GithubSensor.poke` and within that method to `self.result_processor`.
Within the GithubTagSensor the `tag_checker` method now pulls the `tag_name` from this passed dictionary instead of using self.tag_name, which was retrieving the template-string before.
I also added `(BaseGithubRepositorySensor can't be used directly to create Airflow tasks)` to the AirflowException raised when using the BaseGithubRepositorySensor directly in a DAG, because the `Override me.` message alone might be confusing for new Airflow users accidentally using this sensor directly.
I'm of course very happy to take suggestions on how to better solve this (and better naming 😅 ). I did not make any changes to the test running on the GitHubTagSensor yet because I am not sure how to mock templating in a unittest.
--
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] TJaniF commented on pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "TJaniF (via GitHub)" <gi...@apache.org>.
TJaniF commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1519540679
Thank you @eladkal! I removed the patch version bump and the changelog entry. :)
--
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] eladkal commented on pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1453369527
>THe breakage sounds reasonable
so lets do a major release.
@TJaniF please add 3.0.0 entry to provider yaml:
https://github.com/apache/airflow/blob/main/airflow/providers/github/provider.yaml#L32
also please add changelog entry with breaking change under 3.0.0 version in
https://github.com/apache/airflow/blob/main/airflow/providers/github/CHANGELOG.rst
You can see example here:
https://github.com/apache/airflow/blob/main/airflow/providers/amazon/CHANGELOG.rst#700
--
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] TJaniF commented on pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "TJaniF (via GitHub)" <gi...@apache.org>.
TJaniF commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1506627773
Gentle poke @eladkal and @uranusjr :)
Is there something else I should add to this PR/ should be doing?
--
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] TJaniF commented on pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "TJaniF (via GitHub)" <gi...@apache.org>.
TJaniF commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1591203803
Note: I did not forget about this, just trying to find the time to figure out enough about testing to add the requested tests :) and to remember/retest what I did here in the first place 👀
Sorry about taking ages, its been a busy month 😅
--
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 a diff in pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #29842:
URL: https://github.com/apache/airflow/pull/29842#discussion_r1174981379
##########
airflow/providers/github/sensors/github.py:
##########
@@ -46,21 +48,32 @@ def __init__(
github_conn_id: str = "github_default",
method_params: dict | None = None,
result_processor: Callable | None = None,
+ allow_templates_in_result_processor: bool = True,
**kwargs,
) -> None:
super().__init__(**kwargs)
self.github_conn_id = github_conn_id
- self.result_processor = None
- if result_processor is not None:
- self.result_processor = result_processor
+ self.result_processor = result_processor
+ self.allow_templates_in_result_processor = allow_templates_in_result_processor
self.method_name = method_name
self.method_params = method_params
- def poke(self, context: Context) -> bool:
+ def poke(self, context: Context, templated_fields: dict | None = None) -> bool:
Review Comment:
Question: Maybe I do not understand someting, but how do you plan to use the "templated_fields" here? From what I understand, `poke` method is called by airflow without those optional "templated_fields", and when you use sensor in your DAG "as is" you cannot really pass those templated_fields here..
Am I missing something?
--
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 pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1453008723
THe breakage sounds reasonable if we bump the GitHub provider to 3.0.0. So the question would be, is having the kwarg default to true a good idea? Let’s bump if it is.
--
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 pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1469671451
> Users who are using the GitHubSensor directly and pass a `result_processor` function directly would either need to turn `allow_templates_in_result_processor` to False or add a `templated_fields` kwarg to their result_processor function. That is probably affecting more users than custom operators and it seems like it could be potentially confusing.
Only if they use inputs that look like templates but actually are not, I think? Because otherwise the input can still be template-rendered and would not change anyway. (It might be ever so slightly slower but shouldn’t break.) That feels like a very specific edge case to me.
--
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] TJaniF commented on pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "TJaniF (via GitHub)" <gi...@apache.org>.
TJaniF commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1451881773
@uranusjr if I change the default of `allow_templates_in_result_processor` to True in the GitHubSensor that means everyone who uses a custom operator using GitHubSensor.poke() with a `result_processor` function needs to have the `templated_fields` kwarg in that function. At least in my understanding this would be the only thing that breaks.
For example this is the error that happens of the current GitHubTagSensor inherits from the BaseGithubRepositorySensor/GitHubSensor with the changes.
```text
[2023-03-02, 13:35:31 UTC] {taskinstance.py:1768} ERROR - Task failed with exception
Traceback (most recent call last):
File "/usr/local/lib/python3.9/site-packages/airflow/sensors/base.py", line 199, in execute
poke_return = self.poke(context)
File "/usr/local/airflow/include/custom_githubtagsensor.py", line 196, in poke
return GithubSensor.poke(self, context=context)
File "/usr/local/airflow/include/custom_githubtagsensor.py", line 68, in poke
return self.result_processor(github_result, templated_fields=templated_fields)
TypeError: tag_checker() got an unexpected keyword argument 'templated_fields'
```
--
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] TJaniF commented on pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "TJaniF (via GitHub)" <gi...@apache.org>.
TJaniF commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1472255381
Thank you @uranusjr ! I made the changes, now `allow_templates_in_result_processor=True` is the default and there should not be any breaking changes. :)
--
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] boring-cyborg[bot] commented on pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1450418626
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)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] TJaniF commented on pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "TJaniF (via GitHub)" <gi...@apache.org>.
TJaniF commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1453595438
@uranusjr @eladkal I just realized there is a second thing to consider if the default for `allow_templates_in_result_processor` is set to True.
Users who are using the GitHubSensor directly and pass a `result_processor` function directly would either need to turn `allow_templates_in_result_processor` to False or add a `templated_fields` kwarg to their result_processor function. That is probably affecting more users than custom operators and it seems like it could be potentially confusing.
I have an idea for a solution that would not break any current direct use of the GithubSensor while making the GitHubTagSensor `tag_name` parameter templateable by default:
Having two separate params for the result processor function, one to use when using the sensor directly (the current one `result_processor`, when there will be no templated inputs to that function and a new one `templated_processor` to use in operators inheriting from the GithubSensor.
I think this change would not be breaking anything, at least what I can think of.
I pushed that version now to compare, let me know which version you think works better. :)
--
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] TJaniF commented on pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "TJaniF (via GitHub)" <gi...@apache.org>.
TJaniF commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1655508854
Closing this because at this point it is far behind main and I expect it will take a while until I have time to sit down and properly learn how to write tests for this and assess if this is really the best of of solving it, sorry about that! 😓 I'll re-PR it once I do and link to this PR.
--
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] TJaniF commented on pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "TJaniF (via GitHub)" <gi...@apache.org>.
TJaniF commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1471013878
> > Users who are using the GitHubSensor directly and pass a `result_processor` function directly would either need to turn `allow_templates_in_result_processor` to False or add a `templated_fields` kwarg to their result_processor function. That is probably affecting more users than custom operators and it seems like it could be potentially confusing.
>
> Only if they use inputs that look like templates but actually are not, I think? Because otherwise the input can still be template-rendered and would not change anyway. (It might be ever so slightly slower but shouldn’t break.) That feels like a very specific edge case to me.
I'm sorry I think I explained this poorly. 😅
I think the issue is that when `allow_templates_in_result_processor=True` in the GithubSensor, the function passed to result_processor is expected to have a parameter called `templated_fields`.
With the first version of this PR (commit [fe97f18](https://github.com/apache/airflow/pull/29842/commits/fe97f1808a557b195dd341c8fefb5d87e82d9af3) the following task fails:
```python
def max_10_branches(repo):
result = None
try:
if repo is not None:
branches = repo.get_branches()
if branches.totalCount <= 10:
result = True
else:
result = False
except Exception as e:
raise AirflowException(f"Github sensor error: {str(e)}")
return result
using_first_version_githubsensor = FIRST_VERSION_GithubSensor(
task_id="first_version_sensor",
github_conn_id='github_default',
method_name="get_repo",
method_params={'full_name_or_id': GITHUB_REPOSITORY},
result_processor=lambda repo: max_10_branches(repo),
allow_templates_in_result_processor=True
)
```
with
```text
[2023-03-15, 23:57:30 UTC] {taskinstance.py:1768} ERROR - Task failed with exception
Traceback (most recent call last):
File "/usr/local/lib/python3.9/site-packages/airflow/sensors/base.py", line 199, in execute
poke_return = self.poke(context)
File "/usr/local/airflow/include/custom_githubtagsensor_old.py", line 68, in poke
return self.result_processor(github_result, templated_fields=templated_fields)
TypeError: <lambda>() got an unexpected keyword argument 'templated_fields'
```
this is why I think setting `allow_templates_in_result_processor=True` in the GithubSensor could lead to an odd pattern of having to include this kwarg in any function passed to `result_processor`.
The current version of the PR prevents this from happening and means the GithubSensor can be used exactly the same as it is right now, the only change is that operators inheriting from it can now use templated args in their processor function if the parameter `templated_processor` is used instead of `result_processor`. As far as I can tell we get the same functionality without any breakage.
--
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] TJaniF closed pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "TJaniF (via GitHub)" <gi...@apache.org>.
TJaniF closed pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
URL: https://github.com/apache/airflow/pull/29842
--
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 #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1583653683
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] eladkal commented on a diff in pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29842:
URL: https://github.com/apache/airflow/pull/29842#discussion_r1173889226
##########
airflow/providers/github/CHANGELOG.rst:
##########
@@ -20,6 +20,14 @@
Changelog
---------
+2.2.2
+.....
+
+Bug Fixes
+~~~~~~~~~
+
+* ``Allow for 'tag_name' in GithubTagSensor to be templated (#29842)``
+
Review Comment:
We are adding manual entry to changelog only in breaking changes (as in that case the commit message is not enough to explain what action users should take) since now the change is backward compatible you can remove this
--
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] eladkal commented on a diff in pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29842:
URL: https://github.com/apache/airflow/pull/29842#discussion_r1173889226
##########
airflow/providers/github/CHANGELOG.rst:
##########
@@ -20,6 +20,14 @@
Changelog
---------
+2.2.2
+.....
+
+Bug Fixes
+~~~~~~~~~
+
+* ``Allow for 'tag_name' in GithubTagSensor to be templated (#29842)``
+
Review Comment:
We are adding changes to change log only in breaking changes (as in that case the commit message is not enough to explain what action users should take) since now the change is backward compatible you can remove this
##########
airflow/providers/github/provider.yaml:
##########
@@ -30,6 +30,7 @@ dependencies:
suspended: false
versions:
+ - 2.2.2
Review Comment:
also remove this
--
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] TJaniF commented on a diff in pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "TJaniF (via GitHub)" <gi...@apache.org>.
TJaniF commented on code in PR #29842:
URL: https://github.com/apache/airflow/pull/29842#discussion_r1152503500
##########
airflow/providers/github/sensors/github.py:
##########
@@ -46,21 +48,28 @@ def __init__(
github_conn_id: str = "github_default",
method_params: dict | None = None,
result_processor: Callable | None = None,
+ allow_templates_in_result_processor: bool = True,
**kwargs,
) -> None:
super().__init__(**kwargs)
self.github_conn_id = github_conn_id
- self.result_processor = None
- if result_processor is not None:
- self.result_processor = result_processor
+ self.result_processor = result_processor
+ self.allow_templates_in_result_processor = allow_templates_in_result_processor
self.method_name = method_name
self.method_params = method_params
- def poke(self, context: Context) -> bool:
+ def poke(self, context: Context, templated_fields: dict | None = None) -> bool:
hook = GithubHook(github_conn_id=self.github_conn_id)
github_result = getattr(hook.client, self.method_name)(**self.method_params)
if self.result_processor:
+ argspec = inspect.getfullargspec(self.result_processor)
+ if self.allow_templates_in_result_processor and (
+ "templated_fields" in argspec.kwonlyargs or "templated_fields" in argspec.args
+ ):
+ return self.result_processor(github_result, templated_fields=templated_fields)
+ if self.allow_templates_in_result_processor:
+ self.log.info("To use templated fields in your `result_processor` function, provide them as a dict to a `templated_fields` parameter in your `result_processor` function.")
Review Comment:
Thank you!
I did the changelog and provider.yaml entry now for a 2.2.2 version since at least as far as I can tell this change should now not break anything :)
Is there anything else I should be doing? (this is my first time committing to Airflow)
--
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 pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1451372967
What are the incompatibilities if this is enabled by default, or always enabled? Since this is from a provider, we can break backward compatibility more freely if we think the new behaviour is significantly better.
--
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] eladkal commented on a diff in pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29842:
URL: https://github.com/apache/airflow/pull/29842#discussion_r1151603260
##########
airflow/providers/github/sensors/github.py:
##########
@@ -46,21 +48,28 @@ def __init__(
github_conn_id: str = "github_default",
method_params: dict | None = None,
result_processor: Callable | None = None,
+ allow_templates_in_result_processor: bool = True,
**kwargs,
) -> None:
super().__init__(**kwargs)
self.github_conn_id = github_conn_id
- self.result_processor = None
- if result_processor is not None:
- self.result_processor = result_processor
+ self.result_processor = result_processor
+ self.allow_templates_in_result_processor = allow_templates_in_result_processor
self.method_name = method_name
self.method_params = method_params
- def poke(self, context: Context) -> bool:
+ def poke(self, context: Context, templated_fields: dict | None = None) -> bool:
hook = GithubHook(github_conn_id=self.github_conn_id)
github_result = getattr(hook.client, self.method_name)(**self.method_params)
if self.result_processor:
+ argspec = inspect.getfullargspec(self.result_processor)
+ if self.allow_templates_in_result_processor and (
+ "templated_fields" in argspec.kwonlyargs or "templated_fields" in argspec.args
+ ):
+ return self.result_processor(github_result, templated_fields=templated_fields)
+ if self.allow_templates_in_result_processor:
+ self.log.info("To use templated fields in your `result_processor` function, provide them as a dict to a `templated_fields` parameter in your `result_processor` function.")
Review Comment:
This line is too long thus failing the static checks
--
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 pull request #29842: Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #29842:
URL: https://github.com/apache/airflow/pull/29842#issuecomment-1471184980
There’s precedence of using `inspect` to check whether a function has a specific argument, and only pass extra values if it is present. We can implement that here so `template_fields` is only passed if the callable defines it as a kwarg.
--
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