[AIRFLOW-2508] Handle non string types in Operators templatized fields #4292

galak75 · 2018-12-07T02:26:59Z

Jira

This PR addresses [AIRFLOW-2508] Handle non string types in render_template_from_field

Description

This PR changes BaseOperator.render_template_from_field method behavior:

it was previously rendering string_types and supporting Numbers (and collections or dictionaries of these types); but it was raising an AirflowException on any other type.
it is now supporting any other type (by returning the value as is)

Tests

This PR adds the following unit tests on BaseOperator.render_template_from_field method:

rendering a list of strings
rendering a tuple of strings
rendering dictionary values
showing dictionary keys are not templatized
returning a date as is
returning a datetime as is
returning a UUID as is
returning an object as is

Notice

This PR adds pyhamcrest to Airflow test dependencies. This module helps writing meaningful assertions, and also provides very clear and helpful messages on test failures.

If Airflow maintainers do not want to include this test module, just let me know, and I'll rework unit tests

Note

This PR was made during working hours thanks to my employer: the city of Montreal

codecov-io · 2019-01-20T23:54:37Z

Codecov Report

Merging #4292 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4292      +/-   ##
==========================================
- Coverage    74.1%   74.03%   -0.08%     
==========================================
  Files         421      421              
  Lines       27681    28059     +378     
==========================================
+ Hits        20514    20773     +259     
- Misses       7167     7286     +119

Impacted Files	Coverage Δ
airflow/models/__init__.py	`92.5% <100%> (+0.16%)`	⬆️
airflow/contrib/hooks/gcp_dataflow_hook.py	`75.75% <0%> (ø)`	⬆️
airflow/bin/cli.py	`66.06% <0%> (+1.41%)`	⬆️
airflow/contrib/hooks/bigquery_hook.py	`58.36% <0%> (+2.34%)`	⬆️
airflow/contrib/hooks/gcp_api_base_hook.py	`81.39% <0%> (+3.78%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d7ece3...d8841ea. Read the comment docs.

galak75 · 2019-01-21T21:15:43Z

I rebased my PR onto master. Is there something more I should do so that this PR can be reviewed ?

ashb · 2019-01-21T21:23:55Z

Could you give an example where this is used in a DAG so we can see it in context?

I also wonder if it is worth noting something about this in the docs?

ashb · 2019-01-21T21:25:02Z

tests/models.py

 from tempfile import NamedTemporaryFile, mkdtemp

 import pendulum
 import six
+from hamcrest import contains, instance_of
+from hamcrest.core import assert_that
+from hamcrest.core.core import is_


Please just use the built in assert methods assertIsIn, assertIsInstanceOf etc.

ashb · 2019-01-21T21:25:50Z

tests/models.py

+            task = DummyOperator(task_id='op1')
+
+        random_uuid = uuid.uuid4()
+        assert_that(task.render_template('', random_uuid, dict(foo='bar')), is_(random_uuid))


Suggested change

assert_that(task.render_template('', random_uuid, dict(foo='bar')), is_(random_uuid))

assert_that(task.render_template('', random_uuid, {'foo':'bar'}), is_(random_uuid))

Elsewhere too - please us {} rather than dict()

ashb

See comments.

galak75 · 2019-01-22T15:15:48Z

Could you give an example where this is used in a DAG so we can see it in context?

An example can be found in AIRFLOW-2508 Jira ticket description. But I can provide an additional one here if required. Just let me know

I also wonder if it is worth noting something about this in the docs?

You're right! My bad, I forgot about it. I will update render_template_from_field method documentation

ashb · 2019-01-22T16:09:29Z

Code and test looks broadly good though. Please ping me once requested changes have been made.

galak75 · 2019-01-22T16:14:04Z

Code and test looks broadly good though. Please ping me once requested changes have been made.

Thank you! And also thank you for reviewing my code!

Quick question : should I squash all my commits together again before submitting requested changes?
Or do you prefer to review these additional commits one by one before?

ashb · 2019-01-22T16:16:21Z

We'll squash before merging in †he GitHub interface so it doesn't make any difference to us.

My personal preference is to see fixup! commits if you are comfortable with that, then I can look at just the fixup changes. But a PR this size I don't mind just a force-push of a single new commit either.

…thods

galak75 · 2019-01-22T19:07:20Z

Hi @ashb, I'm done with my changes.
There is one test in error in Travis, but I clearly do not understand why...

apache#4292)

galak75 force-pushed the AIRFLOW-2508-non-string-types-in-templates branch from b3a597e to 0421823 Compare December 10, 2018 17:00

galak75 force-pushed the AIRFLOW-2508-non-string-types-in-templates branch 3 times, most recently from 776eed7 to ac020a7 Compare January 9, 2019 21:06

galak75 force-pushed the AIRFLOW-2508-non-string-types-in-templates branch from ac020a7 to c1230ef Compare January 20, 2019 23:10

galak75 force-pushed the AIRFLOW-2508-non-string-types-in-templates branch from c1230ef to 798c79e Compare January 21, 2019 15:23

[AIRFLOW-2508] Handle non string types in Operators templatized fields

77e30bc

galak75 force-pushed the AIRFLOW-2508-non-string-types-in-templates branch from 798c79e to 77e30bc Compare January 21, 2019 20:18

ashb reviewed Jan 21, 2019

View reviewed changes

ashb requested changes Jan 22, 2019

View reviewed changes

galak75 added 4 commits January 22, 2019 12:38

[AIRFLOW-2508] : update render_template_from_field documentation

d0d62d8

[AIRFLOW-2508] : remove pyhamcrest dependency, use built-in assert me…

6418b55

…thods

[AIRFLOW-2508] : replace calls to dict() constructor by literals

12ce3e2

[AIRFLOW-2508] : inline calls to render_template() in test assertions

d8841ea

ashb approved these changes Jan 23, 2019

View reviewed changes

ashb merged commit 92727f7 into apache:master Jan 23, 2019

galak75 deleted the AIRFLOW-2508-non-string-types-in-templates branch January 23, 2019 21:31

ashb pushed a commit to ashb/airflow that referenced this pull request Mar 5, 2019

[AIRFLOW-2508] Handle non string types in Operators templatized fields (

057258e

apache#4292)

ashb mentioned this pull request Mar 6, 2019

[AIRFLOW-3948] Operator list as templated field issue #4848

Closed

6 tasks

wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019

[AIRFLOW-2508] Handle non string types in Operators templatized fields (

d8f4028

apache#4292)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-2508] Handle non string types in Operators templatized fields #4292

[AIRFLOW-2508] Handle non string types in Operators templatized fields #4292

galak75 commented Dec 7, 2018 •

edited

Loading

codecov-io commented Jan 20, 2019 •

edited

Loading

galak75 commented Jan 21, 2019

ashb commented Jan 21, 2019

ashb Jan 21, 2019

ashb Jan 21, 2019

ashb Jan 21, 2019

ashb left a comment

galak75 commented Jan 22, 2019

ashb commented Jan 22, 2019

galak75 commented Jan 22, 2019

ashb commented Jan 22, 2019

galak75 commented Jan 22, 2019

	assert_that(task.render_template('', random_uuid, dict(foo='bar')), is_(random_uuid))
	assert_that(task.render_template('', random_uuid, {'foo':'bar'}), is_(random_uuid))

[AIRFLOW-2508] Handle non string types in Operators templatized fields #4292

[AIRFLOW-2508] Handle non string types in Operators templatized fields #4292

Conversation

galak75 commented Dec 7, 2018 • edited Loading

Jira

Description

Tests

Notice

Note

codecov-io commented Jan 20, 2019 • edited Loading

Codecov Report

galak75 commented Jan 21, 2019

ashb commented Jan 21, 2019

ashb Jan 21, 2019

Choose a reason for hiding this comment

ashb Jan 21, 2019

Choose a reason for hiding this comment

ashb Jan 21, 2019

Choose a reason for hiding this comment

ashb left a comment

Choose a reason for hiding this comment

galak75 commented Jan 22, 2019

ashb commented Jan 22, 2019

galak75 commented Jan 22, 2019

ashb commented Jan 22, 2019

galak75 commented Jan 22, 2019

galak75 commented Dec 7, 2018 •

edited

Loading

codecov-io commented Jan 20, 2019 •

edited

Loading