Add conn to jinja template context #16686
Conversation
|
pinging @uranusjr , @potiuk , and @turbaszek as they were involved in the discussion of #14597 |
|
Logic looks good to me, but this won’t pass static checks (formatting issues). Please try to run those locally and fix them first. |
I did run those locally prior to submitting the PR. Also it seems to be passing the static checks in CI too https://github.com/apache/airflow/pull/16686/checks?check_run_id=2929988055 |
|
Yeah. We are just about to remove pylint. Don't worry about that |
|
See #16682 |
ecerulm
force-pushed
the
jinjaconn
branch
2 times, most recently
from
f4dca29
to
57283d4
Compare
github-actions
bot
added
the
full tests needed
|
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. |
|
I did change the connection accessor syntax from Also, I guess I need to do a PR towards airflow-site too? https://github.com/apache/airflow/edit/main/docs/apache-airflow/macros-ref.rst , right? Do I do that already now or we shall wait until this PR is merged. |
Good point. Not really PR to airflow-site - you need to update the https://github.com/apache/airflow/blob/main/docs/apache-airflow/macros-ref.rst in the current PR. The docs at "airflow-site" are generated from there when we release. |
|
Question: how is it going to work for connection custom fields? for example: Asana has |
That should be |
There was a problem hiding this comment.
We should add docs in https://airflow.apache.org/docs/apache-airflow/stable/concepts/connections.html (similar to https://airflow.apache.org/docs/apache-airflow/stable/concepts/variables.html) about getting connections in template.
And in https://airflow.apache.org/docs/apache-airflow/stable/macros-ref.html
The source code for those files are:
|
Ok, I added testcase for |
|
Would you please rebase @ecerulm ? We have merged two stability improvements for CI (pylint + helm race condition). It should help a a bit :) |
Because jinja is flexible this can also be done as
|
airflow/models/taskinstance.py
Outdated
| """ | ||
| Wrapper around Connection. This way you can get connections in | ||
| templates by using ``{{ conn.conn_id }}`` or | ||
| ``{{ conn.get('conn_id', 'fallback') }}``. |
There was a problem hiding this comment.
Does a fallback make sense here? For variables is was easier as it is a "simple" object, but connection is much more complex, so I can't think what we would put in the fallback value to make the template be useful?
(Not saying we should remove it, just that maybe the 'fallback' isn't the best example here.
There was a problem hiding this comment.
Ah, you've got a useful example in the macros doc. Maybe just put a "see macros" here to avoid duplication.
|
(None of my comments are blocking -- if you think they are useful go for it) |
|
@potiuk the CI still fails for reason I believe unrelated to the content of this PR. I can rebase again but judging from the last PR merged https://github.com/apache/airflow/pull/15397/checks which had the same checks failing as this one, I don't think that will help. |
|
Yeah. It looks like a problem with the GitHub (public) runners this time. There seem to be no relation between Python/DB or any other consistent pattern with those failures. Maybe you could apply some of the comments from @ashb and when you do - we merge it finally. |
|
Something fishy going on with helm installation on public runners randomly: |
|
But unrelated. Merging. |
closes: #14597
^ Add meaningful description above
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.