Create Undefined Jinja Variables Rule #11241

ashmeet13 · 2020-10-02T16:11:32Z

Hi,

Adding a rule to check for Undefined Jinja Variables (Issue: #11144) when upgrading to Airflow2.0

Logic - Use a DagBag to pull all dags and iterate over every dag. For every dag the task will be rendered using
an updated Jinja Environment using - jinja2.DebugUndefined. This will render the template leaving undefined variables
as they were. Using regex we can extract the variables and present possible error cases when upgrading.

Since I am fairly new to Airflow - please do guide if there is a better approach to implement this rule.
Thanks!

Related: #8765
Fixes: #11144

boring-cyborg · 2020-10-02T16:11:35Z

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/master/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, pylint 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

potiuk · 2020-10-03T21:05:00Z

Can you please rebase to latest v1-10-test ? It should work now.

potiuk · 2020-10-03T21:05:11Z

@ashmeet13 ^^

ashmeet13 · 2020-10-04T09:00:22Z

Hey @potiuk rebased the branch.
Two of the checks seemed to have failed -

CI Build / Quarantined tests
CI Build / K8s: 3.6 v1.17.5 image

Could you please guide towards how should I be fixing them?

potiuk · 2020-10-04T09:19:53Z

The quarantined tests are kind and f ok to fail. Those are flaky tests that we know are not stable. The k8s ones are likely a transient error . So do not worry about those

ashmeet13 · 2020-10-05T04:34:26Z

Got it @potiuk. Thanks for letting me now.

ashmeet13 · 2020-10-06T13:04:08Z

Hey @potiuk, made a few changes in my branch today and rebased to latest v1-10-test. Seeing a lot more failed/cancelled tests.
What should I be doing?

Also - who do I request a review from?
Thank you!

potiuk · 2020-10-06T18:02:11Z

Not sure what's going on here but there is at least one commit that should not be there from what I see (the first one). So something's going on with this (and I See some strange/intermittent errors). can you please try to rebase again on top of v1-10-test and push ? v1-10-test seems to build all right.

ashmeet13 · 2020-10-07T06:32:54Z

I think I have messed up the branch.
I see that this commit - Conditional MySQL Client installation was included but should not have been here.

When I try to rebase again as you suggested it continues to Applying: Conditional MySQL Client installation which is a commit I do not know how I have managed to make here.

Not really sure what to do. Sorry for the trouble. What would your suggestion be to do?

github-actions · 2020-10-08T06:44:48Z

The Build Workflow run is cancelling this PR. It in earlier duplicate of 1029499 run.

ashmeet13 · 2020-10-08T07:31:26Z

Hey @potiuk - took a while but I believe the branch is fixed now.
Also the PR did get closed in the middle when trying to fix the branch. My bad on screwing up the rebase and fixing the branch.

Marking the branch ready for review

mik-laj · 2020-10-19T19:18:47Z

airflow/upgrade/rules/undefined_jinja_varaibles.py

+
+        return messages
+
+    def _dag_level_(self, dag):


I don't understand the name of this method. Could you use a more descriptive name?

Fixing this with a better name.

Fixed names in 08b0f75

mik-laj · 2020-10-19T19:19:33Z

airflow/upgrade/rules/undefined_jinja_varaibles.py

+        dags = dagbag.dags
+        messages = []
+        for dag_id, dag in dags.items():
+            dag_messages = self._dag_level_(dag)


Do you think it is worth omitting checking some DAGs in some cases?

Didn't exactly get this?
Which DAGs could be omitted from checking?

DAGs in which the user has decided about a specific behavior should not be checked.

with DAG(dag_id="case_a"):

with DAG(dag_id="case_b", template_undefined=jinja2.StrictUndefined):

with DAG(dag_id="case_c", template_undefined=jinja2.DebugUndefined):

with DAG(dag_id="case_d", template_undefined=jinja2.Undefined):

Only dag_a should be checked. The others should be omitted as they will work properly in Airlfow 2.0.

Any progress?

I missed this comment - my fault.
How exactly would we check this?

My thought process -
This would require the check to read the Python scripts in which the DAGs have been defined. Then read line by line where the DAG is defined and then check whether template_undefined is defined or not.

It seems to me that it is only possible if we make changes to the DAG constructor and get_template_env method.

airflow/airflow/models/dag.py

Line 238 in d8aa24b

template_undefined: Type[jinja2.StrictUndefined] = jinja2.StrictUndefined,

- template_undefined: Type[jinja2.StrictUndefined] = jinja2.StrictUndefined, + template_undefined: Optional[Type[jinja2.StrictUndefined]] = None,

airflow/airflow/models/dag.py

Line 968 in d8aa24b

'undefined': self.template_undefined,

- 'undefined': self.template_undefined, + 'undefined': self.template_undefined or jinja2.StrictUndefined,,

After making the above changes, we can check the class attribute to test our condition without any major problems.

I see. Makes sense.
I believe this would require a new PR for the change?

Should I raise one for the same?

Why does this require new PR? I think you can update this PR.

Got it. Working on the changes.

Added the changes w.r.t to v1-10-stable as the target branch.
The code changes you had suggested were w.r.t to master branch.

Also added the logic to skip check for DAG when template_undefined is explicitly mentioned

mik-laj · 2020-10-19T19:27:18Z

airflow/upgrade/rules/undefined_jinja_varaibles.py

+            for key, value in rendered_content.items():
+                debug_error_messages.union(self._check_rendered_content(str(value)))
+            return debug_error_messages
+


What is the expected behavior in the "else" case?

My bad. Got neglected from my end. Working on adding this logic.

Added the logic in 08b0f75

kaxil · 2020-10-29T13:20:43Z

@turbaszek I have changed the base to v1-10-stable and rebased it accordingly.
There are two checks that seem to be failing -

CI Build / Core:Sqlite Py3.8 (pull_request)

CI Build / Quarantined:Pg9.6,Py3.6 (pull_request)

What should be done to fix them?

Ignore the Quarantined one, core:Sqlite one seems to be just flaky so that can be ignored too

mik-laj · 2020-10-29T13:35:13Z

airflow/upgrade/rules/undefined_jinja_varaibles.py

+    title = "Jinja Template Variables cannot be undefined"
+
+    description = """\
+Jinja Templates have been updated to the following rule - jinja2.StrictUndefined


Here it should be written that the user should correct the templates or pass template_undefined=jinja2.Undefined to the DAG.

Got it. Added these instructions in description

mik-laj · 2020-10-29T13:37:28Z

airflow/upgrade/rules/undefined_jinja_varaibles.py

+    title = "Jinja Template Variables cannot be undefined"
+
+    description = """\
+Jinja Templates have been updated to the following rule - jinja2.StrictUndefined


Suggested change

Jinja Templates have been updated to the following rule - jinja2.StrictUndefined

The default behavior for DAG's Jinja templates has changed. Now, more restrictive validation of non-existent variables is applied - `jinja2.StrictUndefined`.

Added this suggestion

github-actions · 2020-10-30T09:49:12Z

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

mik-laj · 2020-11-04T23:17:46Z

@turbaszek Can you look at it?

ashmeet13 · 2020-11-10T10:29:44Z

@turbaszek requesting for a review whenever you get the chance.
Thanks!

turbaszek · 2020-11-17T10:10:10Z

@ashmeet13 this change looks good to me, can you please squash the commits and the rebase onto current v1-10-test?

ashmeet13 · 2020-11-17T10:25:43Z

Yup. Just to confirm what exactly needs to be done?
Do I change the target branch back to v1-10-test and then rebase my commits with v1-10-test?

Could you clarify @turbaszek

turbaszek · 2020-11-17T10:40:30Z

Yup. Just to confirm what exactly needs to be done?
Do I change the target branch back to v1-10-test and then rebase my commits with v1-10-test?

Could you clarify @turbaszek

Sorry my bad! I meant v1-10-stable as it is now. The was fixed so it would be nice to:

squash all your commits form this branch (squashing will help with rebasing)
rebase it onto v1-10-stable branch

In this way we will be able to run whole CI tests hopefully

potiuk · 2020-11-17T13:53:33Z

I guess that was a bad rebase :) @ashmeet13 ? Seems that it is going to be better now.

ashmeet13 · 2020-11-17T14:07:52Z

Yup.
Figured it out. Just saw #development - A PR from v1-10-test was merged.
Rebased. Hopefully this works. My bad for the confusion!

ashmeet13 · 2020-11-17T14:43:39Z

The checks have passed. Anything else needed to be done @turbaszek ?

Adding a rule to check for undefined jinja variables when upgrading to Airflow2.0

ashmeet13 · 2020-11-19T11:01:04Z

Can this PR be merged @turbaszek @potiuk if no other changes required?

boring-cyborg · 2020-11-19T11:03:09Z

Awesome work, congrats on your first merged pull request!

Adding a rule to check for undefined jinja variables when upgrading to Airflow2.0 (cherry picked from commit 18100a0)

) Adding a rule to check for undefined jinja variables when upgrading to Airflow2.0 (cherry picked from commit 18100a0)

turbaszek added the area:upgrade Facilitating migration to a newer version of Airflow label Oct 3, 2020

potiuk linked an issue Oct 3, 2020 that may be closed by this pull request

Create UndefinedJinjaVariablesRule to ease upgrade to 2.0 #11144

Closed

potiuk force-pushed the v1-10-test branch from c38c3e5 to 2474f74 Compare October 3, 2020 19:47

potiuk force-pushed the v1-10-test branch from bd4214b to 195f00d Compare October 3, 2020 22:42

ashmeet13 force-pushed the undefined-jinja-variable-rule branch from f08f4c7 to 60d021a Compare October 6, 2020 10:26

ashmeet13 marked this pull request as draft October 7, 2020 15:18

ashmeet13 force-pushed the undefined-jinja-variable-rule branch from 60d021a to fcb3b8d Compare October 7, 2020 15:24

ashmeet13 closed this Oct 8, 2020

ashmeet13 force-pushed the undefined-jinja-variable-rule branch from fcb3b8d to 25b6cb0 Compare October 8, 2020 06:00

ashmeet13 reopened this Oct 8, 2020

ashmeet13 marked this pull request as ready for review October 8, 2020 07:31

turbaszek requested review from turbaszek and mik-laj October 10, 2020 11:14

mik-laj reviewed Oct 19, 2020

View reviewed changes

ashmeet13 requested a review from mik-laj October 21, 2020 13:54

ashmeet13 closed this Oct 23, 2020

ashmeet13 force-pushed the undefined-jinja-variable-rule branch from 097e383 to 3edc89c Compare October 23, 2020 15:19

mik-laj reviewed Oct 29, 2020

View reviewed changes

ashmeet13 requested a review from mik-laj October 31, 2020 05:18

mik-laj approved these changes Nov 4, 2020

View reviewed changes

potiuk force-pushed the v1-10-stable branch from 4d499b4 to b8f3d93 Compare November 13, 2020 13:26

kaxil force-pushed the v1-10-stable branch from b8f3d93 to 005a615 Compare November 13, 2020 13:56

ashmeet13 force-pushed the undefined-jinja-variable-rule branch from 6a128da to 2d509cf Compare November 17, 2020 07:12

turbaszek approved these changes Nov 17, 2020

View reviewed changes

ashmeet13 force-pushed the undefined-jinja-variable-rule branch 2 times, most recently from 5017fdc to 5ef4259 Compare November 17, 2020 13:46

Create UndefinedJinjaVariablesRule (Resolves apache#11144)

ee2ccf7

Adding a rule to check for undefined jinja variables when upgrading to Airflow2.0

ashmeet13 force-pushed the undefined-jinja-variable-rule branch from 5ef4259 to ee2ccf7 Compare November 19, 2020 09:23

turbaszek approved these changes Nov 19, 2020

View reviewed changes

turbaszek merged commit 18100a0 into apache:v1-10-stable Nov 19, 2020

turbaszek mentioned this pull request Nov 19, 2020

Create UndefinedJinjaVariablesRule to ease upgrade to 2.0 #11144

Closed

kaxil pushed a commit that referenced this pull request Nov 20, 2020

Create UndefinedJinjaVariablesRule (Resolves #11144) (#11241)

415cda4

Adding a rule to check for undefined jinja variables when upgrading to Airflow2.0 (cherry picked from commit 18100a0)

kaxil pushed a commit that referenced this pull request Nov 21, 2020

Create UndefinedJinjaVariablesRule (Resolves #11144) (#11241)

15b628e

Adding a rule to check for undefined jinja variables when upgrading to Airflow2.0 (cherry picked from commit 18100a0)

	Jinja Templates have been updated to the following rule - jinja2.StrictUndefined
	The default behavior for DAG's Jinja templates has changed. Now, more restrictive validation of non-existent variables is applied - `jinja2.StrictUndefined`.

Create Undefined Jinja Variables Rule #11241

Create Undefined Jinja Variables Rule #11241

Conversation

ashmeet13 commented Oct 2, 2020 • edited by potiuk Loading

boring-cyborg bot commented Oct 2, 2020

potiuk commented Oct 3, 2020

potiuk commented Oct 3, 2020

ashmeet13 commented Oct 4, 2020

potiuk commented Oct 4, 2020

ashmeet13 commented Oct 5, 2020

ashmeet13 commented Oct 6, 2020

potiuk commented Oct 6, 2020

ashmeet13 commented Oct 7, 2020 • edited Loading

github-actions bot commented Oct 8, 2020

ashmeet13 commented Oct 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-laj Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaxil commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 30, 2020

mik-laj commented Nov 4, 2020

ashmeet13 commented Nov 10, 2020

turbaszek commented Nov 17, 2020

ashmeet13 commented Nov 17, 2020

turbaszek commented Nov 17, 2020

potiuk commented Nov 17, 2020

ashmeet13 commented Nov 17, 2020

ashmeet13 commented Nov 17, 2020

ashmeet13 commented Nov 19, 2020

boring-cyborg bot commented Nov 19, 2020

ashmeet13 commented Oct 2, 2020 •

edited by potiuk

Loading

ashmeet13 commented Oct 7, 2020 •

edited

Loading

mik-laj Oct 19, 2020 •

edited

Loading