Allow for templated args to be passed to the result_processor function in the GithubSensor fixing templated tags in GithubTagSensor

[TJaniF]

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:

[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.

[boring-cyborg]

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 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 Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment 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 for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
  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

[uranusjr]

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.

[TJaniF]

@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 if the current GitHubTagSensor inherits from the BaseGithubRepositorySensor/GitHubSensor with the changes.

[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'

[uranusjr]

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.

[eladkal]

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

[TJaniF]

@uranusjr @eladkal Thank you for the reviews!

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. :)

[uranusjr]

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.

[TJaniF]

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 the following task fails:

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

[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 by default in the GithubSensor could lead to an odd pattern of having to include a templated_fields parameter 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.

[uranusjr]

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.

[TJaniF]

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. :)

[eladkal]

This line is too long thus failing the static checks

[TJaniF]

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)

[TJaniF]

Gentle poke @eladkal and @uranusjr :)
Is there something else I should add to this PR/ should be doing?

[eladkal]

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

[eladkal]

also remove this

[TJaniF]

Thank you @eladkal! I removed the patch version bump and the changelog entry. :)

[potiuk]

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?

[potiuk]

I have a question about the usage of the sensor - I cannot understand how the new parameter will be used.

Also - regardless - this PR does not have any tests and it adds substantial code, this gives no regression protection capabilities so it needs some tests to be mergable.

[github-actions]

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.

[TJaniF]

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 😅

[TJaniF]

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.