You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/01/12 18:28:28 UTC
[GitHub] [airflow] potiuk opened a new pull request #13640: Fixes problems with extras for custom connection types
potiuk opened a new pull request #13640:
URL: https://github.com/apache/airflow/pull/13640
The custom providers with custom connections can define
extra widgets and fields, however there were problems with
those custom fields in Aiflow 2.0.0:
* When connection type was a subset of another connection
type (for example jdbc and jdbcx) widgets from the
'subset' connection type appeared also in the 'superset'
one due to prefix matching in javascript.
* Each connection when saved received 'full set' of extra
fields from other connection types (with empty values).
This problem is likely present in Airflow 1.10 but due
to limited number of connections supported it had no
real implications besides slightly bigger dictionary
stored in 'extra' field.
* The extra field values were not saved for custom connections.
Only the predefined connection types could save extras in
extras field.
This PR fixes it by:
* adding __ matching for javascript to match only full connection
types not prefixes
* saving only the fields matching extra__<conn_type> when the
connection is saved
* removing filtering on 'known' connection types (the above
filtering on `extra__` results in empty extra for
connections that do not have any extra field defined.
Fixes #13597
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759092886
One more try. How about adding `pip uninstall --local 'apache-airflow'` before? This will fail if airflow has not been installed before.
I think the duplication issue is gone because of `jdbx` name so it might be unrelated.
Also - did you actually add the `USER airflow` before pip install ? Maybe you missed the last comment of mine. The prod image has airflow installed for 'airflow' user and thee --user flag uses the ${HOME}/.local of the current user. So it could be that you installed the new airflow for the root user in /root/.local and then when you run the image you run it as 'airflow' user with /home/airflow/.local.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759050386
> This looks good to me, but I can't easily verify because I'm having trouble figuring out how to correctly install from `master` in my setup. I've set myself up a bit unorthodox with a Win10 machine and I develop my provider package with Docker based on the official Python3.8 image. Adding a line like `RUN pip install -e git+https://github.com/apache/airflow.git` in my [Dockerfile](https://github.com/Gemma-Analytics/ewah/blob/master/Dockerfile) breaks the airflow installation :/
Just remove '-e'
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759099578
the `uninstall` doesn't help:
The issue persists. It looks like your PR _should_ work - it makes perfect sense - but nonetheless I cannot seem to get it to work in my Docker image :/ I think I'm not making any progress anymore tonight. Thank you for the patience with me!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759275044
Yep. Confirmed it is your installation.
I just installed `ewah` in my 'master' uncommented JDBC (and modified it to JDBCx) and all works fine. I even was able to edit "ewah" connections (particularly snowflake with extras):
This is output of `airflow connections list` after I added it:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759600092
@soltanianalytics - can you please check the latest `apache/airflow:master-python3.8` . I believe the warnings should be gone.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759074800
> Hmm your line works, the Dockerfile builds successfully and airflow runs. However, the issue is _not_ fixed. I wonder if it's because it's not fixed by this PR or if my installation is not properly updated?
> 
aaaah. you use Prod image as a base....
Just add `--user` flag right after `pip install`.
Longer explanation: In our prod image we use `--user` in order to optimize the image (when you use --user flag, everything is installed to "${HOME}/.local" (including any bin libraries/compiled stuff) and then thee ${HOME}/.local folder can be copied over (we copy it so that no build-essentials like c/c++ compiler are preesent in the final image) that's why our iimage is just 200 MB and not 400MB.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759098491
Yeah I re-add a connection every time I test it - I used various variants for the connection name. However, in the hook code it's 100% consistent:
```Python
class JdXbcHook(DbApiHook):
"""
General hook for jdbc db access.
JDBC URL, username and password will be taken from the predefined connection.
Note that the whole JDBC URL must be specified in the "host" field in the DB.
Raises an airflow error if the given connection id doesn't exist.
"""
conn_name_attr = 'jdxbc_conn_id'
default_conn_name = 'jdxbc_default'
conn_type = 'jdxbc'
hook_name = 'JDxBC Connection'
supports_autocommit = True
@staticmethod
def get_connection_form_widgets() -> Dict[str, Any]:
"""Returns connection widgets to add to connection form"""
from flask_appbuilder.fieldwidgets import BS3TextFieldWidget
from flask_babel import lazy_gettext
from wtforms import StringField
return {
"extra__jdxbc__drv_path": StringField(lazy_gettext('Driver Path'), widget=BS3TextFieldWidget()),
"extra__jdxbc__drv_clsname": StringField(
lazy_gettext('Driver Class'), widget=BS3TextFieldWidget()
),
}
@staticmethod
def get_ui_field_behaviour() -> Dict:
"""Returns custom field behaviour"""
return {
"hidden_fields": ['port', 'schema', 'extra'],
"relabeling": {'host': 'Connection URL'},
}
```
Plus, it does show the widget correctly, it just never saves data
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759082834
This is interesting information, thanks! I haven't tried rebuilding the image before, I will read into it.
On the issue at hand: The `--user` flag didn't change anything, though. Unsurprisingly, since I believe `pip install` in this image defaults to `--user` behavior anyway.
The build command seems to be successful (see dev_build 14/14):
Notably, only the _saving data_ error persists, not the duplicate fields, which suggests that the installation is successful but there might be another issue somewhere?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759098491
Yeah I re-add a connection every time I test it - I used various variants for the connection name. However, in the hook code it's 100% consistent:
```Python
class JdXbcHook(DbApiHook):
"""
General hook for jdbc db access.
JDBC URL, username and password will be taken from the predefined connection.
Note that the whole JDBC URL must be specified in the "host" field in the DB.
Raises an airflow error if the given connection id doesn't exist.
"""
conn_name_attr = 'jdxbc_conn_id'
default_conn_name = 'jdxbc_default'
conn_type = 'jdxbc'
hook_name = 'JDxBC Connection'
supports_autocommit = True
@staticmethod
def get_connection_form_widgets() -> Dict[str, Any]:
"""Returns connection widgets to add to connection form"""
from flask_appbuilder.fieldwidgets import BS3TextFieldWidget
from flask_babel import lazy_gettext
from wtforms import StringField
return {
"extra__jdxbc__drv_path": StringField(lazy_gettext('Driver Path'), widget=BS3TextFieldWidget()),
"extra__jdxbc__drv_clsname": StringField(
lazy_gettext('Driver Class'), widget=BS3TextFieldWidget()
),
}
@staticmethod
def get_ui_field_behaviour() -> Dict:
"""Returns custom field behaviour"""
return {
"hidden_fields": ['port', 'schema', 'extra'],
"relabeling": {'host': 'Connection URL'},
}
```
Plus, it does show the widgets correctly, it just never saves data
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759093631
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759074800
> Hmm your line works, the Dockerfile builds successfully and airflow runs. However, the issue is _not_ fixed. I wonder if it's because it's not fixed by this PR or if my installation is not properly updated?
> 
aaaah. you use Prod image as a base....
Just add `--user` flag right after `pip install`.
Longer explanation: In our prod image we use `--user` in order to optimize the image (when you use --user flag, everything is installed to "${HOME}/.local" (including any bin libraries/compiled stuff) and then thee ${HOME}/.local folder can be copied over (we copy it so that no build-essentials like c/c++ compiler) are preesent in the final image.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759055491
`pip install git+https://github.com/PolideaInternal@fix-extra-fields-behaviour-for-custom-connections` should to. Testing it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759078866
BTW. Side comment - It might be a good idea to add psycopg2 to our image. I saw you are adding it in your image. We have some discussions about GPL-licenced software in our image (in ASF) but having Psycopg2 might be a good idea. I am not sure if you are aware, but you can also customize our image yourself and built psygcopg2 and add extra env-vars by just rebuilding the image with appropriate build args.
This can bee done later when you go to production, but it is fully supported:
You can read more here: http://airflow.apache.org/docs/apache-airflow/stable/production-deployment.html#customizing-or-extending-the-production-image (customizig the image is what you should look for)
Also there is my presentation about it from Airflow Summit: https://www.youtube.com/watch?v=wDr3Y7q2XoI
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on a change in pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on a change in pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#discussion_r556164253
##########
File path: airflow/www/views.py
##########
@@ -2894,9 +2894,13 @@ def action_muldelete(self, items):
def process_form(self, form, is_created):
"""Process form data."""
- formdata = form.data
- if formdata['conn_type'] in ['jdbc', 'google_cloud_platform', 'grpc', 'yandexcloud', 'kubernetes']:
- extra = {key: formdata[key] for key in self.extra_fields if key in formdata}
+ conn_type = form.data['conn_type']
+ extra = {
+ key: form.data[key]
+ for key in self.extra_fields
+ if key in form.data and key.startswith(f"extra__{conn_type}__")
Review comment:
This assumes that the key already exists in `form.data`. (`if key in form.data ...`)
In `prefill_form` below, this may not necessarily the case?
##########
File path: airflow/www/views.py
##########
@@ -2894,9 +2894,13 @@ def action_muldelete(self, items):
def process_form(self, form, is_created):
"""Process form data."""
- formdata = form.data
- if formdata['conn_type'] in ['jdbc', 'google_cloud_platform', 'grpc', 'yandexcloud', 'kubernetes']:
- extra = {key: formdata[key] for key in self.extra_fields if key in formdata}
+ conn_type = form.data['conn_type']
+ extra = {
+ key: form.data[key]
+ for key in self.extra_fields
+ if key in form.data and key.startswith(f"extra__{conn_type}__")
Review comment:
```Python
def prefill_form(self, form, pk):
"""Prefill the form."""
try:
extra = form.data.get('extra')
if extra is None:
extra_dictionary = {}
else:
extra_dictionary = json.loads(extra)
except JSONDecodeError:
extra_dictionary = {}
if not isinstance(extra_dictionary, dict):
logging.warning('extra field for %s is not a dictionary', form.data.get('conn_id', '<unknown>'))
return
for field in self.extra_fields:
value = extra_dictionary.get(field, '')
if value: # !! This only adds data for the form.field if it previously exists in the extra
field = getattr(form, field)
field.data = value
```
-> could it be that hence, the `form.data` does not contain the keys for the extra widgets?
Generally, shouldnt `process_form` and `prefill_form` be analogous? From `prefill_form`, I could imagine something along those lines:
```Python
def process_form(self, form, is_created):
"""Process form data."""
conn_type = form.data['conn_type']
extra = {
key: getattr(form, key).data
for key in self.extra_fields
if hasattr(form, key) and key.startswith(f"extra__{conn_type}__") and getattr(form, key).data
}
if extra.keys():
form.extra.data = json.dumps(extra)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-758896048
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759093631
Yeah, I only have a distinct set of commands I run as root: https://github.com/Gemma-Analytics/ewah/blob/master/Dockerfile#L3-L37
Everything else, including the pip install, runs as airflow.
You're right, the duplication is probably gone due to the renaming.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#discussion_r556056371
##########
File path: airflow/www/views.py
##########
@@ -2894,9 +2894,13 @@ def action_muldelete(self, items):
def process_form(self, form, is_created):
"""Process form data."""
- formdata = form.data
- if formdata['conn_type'] in ['jdbc', 'google_cloud_platform', 'grpc', 'yandexcloud', 'kubernetes']:
- extra = {key: formdata[key] for key in self.extra_fields if key in formdata}
+ conn_type = form.data['conn_type']
+ extra = {
+ key: form.data[key]
+ for key in self.extra_fields
+ if key in form.data and key.startswith(f"extra__{conn_type}")
Review comment:
Oh yeah! Good call :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759091999
Could this be the issue?
shouldn't it be `form.data.extra` instead of `form.extra.data`?
https://github.com/apache/airflow/blob/77f0ca5682017ef50dbf90f84df75362e5da5c8b/airflow/www/views.py#L2904
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759095654
another "last" check - did you change the "extra__jdbcx__" to "extra__jdbx__" in get_connection_form_widgets ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759341177
awesome! I'll try to reproduce it later today as well, but I'm already very glad to see that this is fixed in 2.0.1 :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759537363
Finally, with the Docker image, I can confirm that the issue is resolved on my end as well! Thanks!
Note: The webserver in this image throws a lot of exceptions - it appears to try to import packages that I did not install (and logically, I also don't have the dependencies installed). Probably an artifact of the image, just wanted to let you know.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759068687
Hmm your line works, the Dockerfile builds successfully and airflow runs. However, the issue is _not_ fixed. I wonder if it's because it's not fixed by this PR or if my installation is not properly updated?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759097345
So is it consistent `jdbx` or `jdx` or `jdxbc` :). I think I saw all of those variants :D..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759118818
I think i know what could have happend. If you have "apache-airflow" as requirement for "ewah", then installing "ewah" might pull airflow from PYPI and install it over the installed in the previous step.
In such case the new image will not help but you might either:
1) remove airflow from requirements of ewah
2) or install airflow AGAIN from master after installing ewah
3) install evah with `--no-deps` (but then if it has any other dependencies you must install them separately)
Generally enable verbose output on your docker build. You seem to use BUILDX as default but I very much prefer the standard 'docker build` because then you see what each command does (also I guess you can enable verbose output for your builldx and we can see better what's going on)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759079752
AND important @soltanianalytics -> make sure to run 'pip install --user' as user 'airflow' not root
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759115675
Try this as base and do install anything extra: `apache/airflow:master-python3.8` . I just pushed it from master.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759019466
> I'll let also @soltanianalytics verify the fix for #13597 before the merge :)
This looks good to me, but I can't easily verify because I'm having trouble figuring out how to correctly install from `master` in my setup. I've set myself up a bit unorthodox with a Win10 machine and I develop my provider package with Docker based on the official Python3.8 image. Adding a line like `RUN pip install -e git+https://github.com/apache/airflow.git` in my [Dockerfile](https://github.com/Gemma-Analytics/ewah/blob/master/Dockerfile) breaks the airflow installation :/
Any tips on how to install this fix in the Docker image? Otherwise, I'll have to _hope_ that this works correctly.
I'll install airflow properly on my Linux partition eventually, but that won't happen before the Airflow 2.0.1 release...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759084902
I hadn't noticed it before but the JavaScript-caused duplicate fields error is indeed gone:
The extra field is still not saved:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759098491
Yeah I re-add a connection every time I test it - I used various variants for the connection name. However, in the hook code it's 100% consistent:
```Python
class JdXbcHook(DbApiHook):
"""
General hook for jdbc db access.
JDBC URL, username and password will be taken from the predefined connection.
Note that the whole JDBC URL must be specified in the "host" field in the DB.
Raises an airflow error if the given connection id doesn't exist.
"""
conn_name_attr = 'jdxbc_conn_id'
default_conn_name = 'jdxbc_default'
conn_type = 'jdxbc'
hook_name = 'JDxBC Connection'
supports_autocommit = True
@staticmethod
def get_connection_form_widgets() -> Dict[str, Any]:
"""Returns connection widgets to add to connection form"""
from flask_appbuilder.fieldwidgets import BS3TextFieldWidget
from flask_babel import lazy_gettext
from wtforms import StringField
return {
"extra__jdxbc__drv_path": StringField(lazy_gettext('Driver Path'), widget=BS3TextFieldWidget()),
"extra__jdxbc__drv_clsname": StringField(
lazy_gettext('Driver Class'), widget=BS3TextFieldWidget()
),
}
@staticmethod
def get_ui_field_behaviour() -> Dict:
"""Returns custom field behaviour"""
return {
"hidden_fields": ['port', 'schema', 'extra'],
"relabeling": {'host': 'Connection URL'},
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759118818
I think i know what could have happend. If you have "apache-airflow" as requirement for "ewah", then installing "ewah" might pull airflow from PYPI and install it over the installed in the previous step.
In such case the new image will not help but you might either:
1) remove airflow from requirements of ewah
2) or install airflow AGAIN from master after installing ewah
3) install evah with `--no-deps` (but then if it has any other dependencies you must install them separately)
Generally enable verbose output on your docker build. You seem to use BUILDX as default but I very much prefer the standard 'docker build` because then you see what each command does (also I guess you can enable verbose output for your builldx and we can see better what's going on)
UPDATE: I see you do not have airflow in your setup.py /cfg but stil enabling verbose output during build would be handy (and try the master image)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759068687
Hmm your line works, the Dockerfile build successfully and airflow runs. However, the issue is _not_ fixed. I wonder if it's because it's not fixed by this PR or if my installation is not properly updated?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759055491
`pip install git+https://github.com/PolideaInternal/airflow@fix-extra-fields-behaviour-for-custom-connections` should to. Testing it.
UPDATE: updated the command after testing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759118818
I think i know what could have happend. If you have "apache-airflow" as requirement for "ewah" installing "eweah" might pull airflow from PYPI and install it over the installed in the previous step.
In such case the new image will not help but you might either:
1) remove airflow from requirements of ewah
2) or install airflow AGAIN from master after installing ewah
3) install evah with `--no-deps` (but then if it has any other dependencies you must install them separately)
Generally enable verbose output on your docker build. You seem to use BUILDX as default but I very much prefer the standard 'docker build` because then you see what each command does (also I guess you can enable verbose output for your builldx and we can see better what's going on)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#discussion_r556177228
##########
File path: airflow/www/views.py
##########
@@ -2894,9 +2894,13 @@ def action_muldelete(self, items):
def process_form(self, form, is_created):
"""Process form data."""
- formdata = form.data
- if formdata['conn_type'] in ['jdbc', 'google_cloud_platform', 'grpc', 'yandexcloud', 'kubernetes']:
- extra = {key: formdata[key] for key in self.extra_fields if key in formdata}
+ conn_type = form.data['conn_type']
+ extra = {
+ key: form.data[key]
+ for key in self.extra_fields
+ if key in form.data and key.startswith(f"extra__{conn_type}__")
Review comment:
Nope. That looks ok. Keys in form.data will be there is there are some fields added and you entered some value there (or it will be pre-fiiled) . The value will only be prefilled if the extra is already in the the connection and is not empty. The code looks good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759097345
So is it consisten `jdbx` or `jdx` or `jdxbc` :). I think I saw all of those variants :D..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759277632
Also I took the opportunity to test how it looks like when you remove the pckage and it is also reasonable. Package type is the first on the list (which happens to be Amazon) but all the fields are correctly retrieved and exras show as extras:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759082834
This is interesting information, thanks! I haven't tried building the rebuilding image before, I will read into it.
On the issue at hand: The `--user` flag didn't change anything, though. Unsurprisingly, since I believe `pip install` in this image defaults to `--user` behavior anyway.
The build command seems to be successful (see dev_build 14/14):
Notably, only the _saving data_ error persists, not the duplicate fields, which suggests that the installation is successful but there might be another issue somewhere?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759554149
Hmm. Indeed. I will take a look. I thought it is only when you have editable version of airflow installed as well, but it seems it's not the case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759091999
Could this be the issue?
shouldn't it be `form.data.extra` instead of `form.extra.data`?
https://github.com/apache/airflow/blob/77f0ca5682017ef50dbf90f84df75362e5da5c8b/airflow/www/views.py#L2904
Edit:
On the one hand, that seems to be how it is used in the next function below:
On the other hand, you didn't change that part and it did work for `jdbc`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759096104
> did you change the "extra__jdbcx__" to "extra__jdbx__" in get_connection_form_widgets ?
Good idea, unfortunately "yes"
I'm building with the uninstall command now, let's see :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759096104
> did you change the "extra__jdbcx__" to "extra__jdbx__" in get_connection_form_widgets ?
Good idea, unfortunately "yes"
I'm building with the uninstall command now, let's see :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759088588
Ok. Will take a look tomorrow and try to repro in our dev image.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759654597
> @soltanianalytics - can you please check the latest `apache/airflow:master-python3.8` . I believe the warnings should be gone.
Can confirm!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-758857088
I'll let also @soltanianalytics verify the fix for #13597 before the merge :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759050386
> This looks good to me, but I can't easily verify because I'm having trouble figuring out how to correctly install from `master` in my setup. I've set myself up a bit unorthodox with a Win10 machine and I develop my provider package with Docker based on the official Python3.8 image. Adding a line like `RUN pip install -e git+https://github.com/apache/airflow.git` in my [Dockerfile](https://github.com/Gemma-Analytics/ewah/blob/master/Dockerfile) breaks the airflow installation :/
Just remove '-e' - also not yet merged so you need to specify branch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759074800
> Hmm your line works, the Dockerfile builds successfully and airflow runs. However, the issue is _not_ fixed. I wonder if it's because it's not fixed by this PR or if my installation is not properly updated?
> 
aaaah. you use Prod image as a base....
Just add `--user` flag right after `pip install`.
Longer explanation: In our prod image we use `--user` in order to optimize the image (when you use --user flag, everything is installed to "${HOME}/.local" (including any bin libraries/compiled stuff) and then thee ${HOME}/.local folder can be copied over (we copy it so that no build-essentials like c/c++ compiler are preesent in the final image and some other stuff) - that's why our image is just 200 MB and not 400MB.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #13640:
URL: https://github.com/apache/airflow/pull/13640
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759124793
One other thing you could do is to see if you have everything installed
You can also check if your evah package is installed and whether the code is as you expect it to be in similar way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] soltanianalytics commented on a change in pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
soltanianalytics commented on a change in pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#discussion_r556025425
##########
File path: airflow/www/views.py
##########
@@ -2894,9 +2894,13 @@ def action_muldelete(self, items):
def process_form(self, form, is_created):
"""Process form data."""
- formdata = form.data
- if formdata['conn_type'] in ['jdbc', 'google_cloud_platform', 'grpc', 'yandexcloud', 'kubernetes']:
- extra = {key: formdata[key] for key in self.extra_fields if key in formdata}
+ conn_type = form.data['conn_type']
+ extra = {
+ key: form.data[key]
+ for key in self.extra_fields
+ if key in form.data and key.startswith(f"extra__{conn_type}")
Review comment:
Like in the `connection_form.js`, adding `__` at the end here avoids the (unlikely) case of substrings in the conn types, like so:
```Python
if key in form.data and key.startswith(f"extra__{conn_type}__")
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759556643
Seems like there is an error somewhere in image build scripts. I will take a very close look at that :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759092886
One more try. How about adding `pip uninstall --local 'apache-airflow'` before? This will fail if airflow has not been installed before.
I think the duplication issue is gone because of `jdbx` name so it might be unrelated.
I think the duplication issue is gone because of `jdbx` name so it might be unrelated. Also - did you actually add the `USER airflow` before pip install ? Maybe you missed the last comment of mine. The prod image has airflow installed for 'airflow' user and thee --user flag uses the ${HOME}/.local of the current user. So it could be that you installed the new airflow for the root user in /root/.local and then when you run the image you run it as 'airflow' user with /home/airflow/.local.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #13640: Fixes problems with extras for custom connection types
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13640:
URL: https://github.com/apache/airflow/pull/13640#issuecomment-759103517
I did a quick test before going to bed and renamed jdbc values and it all works - so I am quite certain there is an installaton problem:
I wil merge it (there were some transient errors) and will make sure that the master prod image is pushe tomorrow so that you can test it this way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org