Jinja templates should be rendered in dict keys

[zbstof]

Currently keys stay unrendered which is confusing for the users and require workaround like explicitly using Variable.get, which is evaluated eagerly during dag rendering, leading to further need for workarounds.

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

[potiuk]

I wonder if it can have some unintended side effects, I cannot think of any, but maybe there was a reason it was implemented like that ?

In any case @zbstof - you will need to add test cases covering that

[potiuk]

We need to have some test cases for that

[zbstof]

Turns out there's already a test that dict keys are not rendered, added by @BasPH , but I think it was codifying existing behaviour, not enforcing a contract.
I fixed that test
Please clarify if you want me to add more test cases?

maybe there was a reason it was implemented like that ?

Given how dicts are just fancy list of 2-tuples, and we already handle explicit lists and tuples, I don't see why we shouldn't handle dictionary keys

[potiuk]

Wel. The fact that there was a test for that indicates that there could be a reason. I cannot think of any as well, but It's better to check it. This is quite crucial part of the code, so I would not like to spoil something.

@BasPH, @kaxil - I'd love to hear your thoughts here. Maybe there is a good reason for not rendering the keys?

[BasPH]

It wasn't me :) This was added a long time ago -> https://github.com/apache/airflow/pull/4292/files

Technically I don't see anything wrong with also templating the keys 👍

[potiuk]

Static checks are failing - the other failing tests is irrelevant - flaky test that will be fixed by #18947

[github-actions]

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

[potiuk]

Works for me then - some static checks need to be fixed though (and I think we should hold-off with merging that until we release 2.2.1 as there are potential conflicts I think).

[zbstof]

I've fixed static check, should I rebase to fix broken tests in Quarantined tests?

[uranusjr]

That would be best (although I don't think it'd help; Quarantined failed due to the task being killed because it ran for too long, not any concrete test failures)

[kaxil]

This is a change in behaviour for keys, can you add this change in UPDATING.md please?

[eladkal]

@zbstof can you please address above comment and rebase?

[zbstof]

@eladkal, @kaxil
I've added an entry in UPDATING.md, please check if it's good enough

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

[zbstof]

Ping нд, 13 лют. 2022, 02:08 користувач github-actions[bot] < ***@***.***> пише:

…

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. — Reply to this email directly, view it on GitHub <#18938 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKPUA5YQLUDFE7A4BFSBQLU23ZBLANCNFSM5F465FCQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

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

[zbstof]

Can some of you point me to the example of the flag that @potiuk suggested?

[potiuk]

Can some of you point me to the example of the flag that @potiuk suggested?

Just a new config like all the other airflow configs described here: https://github.com/apache/airflow/blob/main/airflow/config_templates/config.yml

[potiuk]

Any news there @zbystof ?

[zbstof]

Something like this, please check. Although due to #22003, changes to UPDATING.md are lost
Now this functionality is hidden by a flag

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

[potiuk]

Conflicts need to be fixed - can you fix them please @zbstof ?

[zbstof]

Fixed conflicts and rebased

[potiuk]

But the tests are failing.

[eladkal]

Tests are still failing

tests/models/test_baseoperator.py::TestBaseOperator::test_render_template_with_flag: AssertionError: assert {'key_2': 'ba... foo }}_1': 1} == {'key_2': 'ba...key_bar_1': 1}
  Omitting 1 identical items, use -vv to show
  Left contains 1 more item:
  {'key_{{ foo }}_1': 1}
  Right contains 1 more item:
  {'key_bar_1': 1}

[zbstof]

I'm going to ask for a bit of help here - why my test does not mock flag value here? Or maybe I'm approaching the test in a completely wrong way?

[potiuk]

Typical problem (been there done that) when you are not used to patching. See the "patching in the wrong place": https://alexmarandon.com/articles/python_mock_gotchas/

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