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 2020/12/05 16:02:06 UTC
[GitHub] [airflow] potiuk opened a new pull request #12837: Add per provider javascript
potiuk opened a new pull request #12837:
URL: https://github.com/apache/airflow/pull/12837
<!--
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 commented on a change in pull request #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537081332
##########
File path: airflow/providers/apache/spark/hooks/spark_submit.py
##########
@@ -109,6 +111,17 @@ class SparkSubmitHook(BaseHook, LoggingMixin):
conn_type = 'spark'
hook_name = 'Spark'
+ @staticmethod
+ def get_connection_form_widgets() -> Dict[str, Field]:
+ return {}
+
+ @staticmethod
+ def get_custom_fields() -> Dict:
Review comment:
No problem with that name.
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537087309
##########
File path: airflow/www/static/js/connection_form.js
##########
@@ -87,7 +34,7 @@ $(document).ready(function () {
$(this).text($(this).attr("orig_text"));
});
$(".form-control").each(function(){$(this).attr('placeholder', '')});
-
+ let config = JSON.parse($("#field_parameters").text())
Review comment:
We really need to fail webserver when assets are not "rebuilt"
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537080688
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -303,7 +303,7 @@ def web_identity_token_loader():
return web_identity_token_loader
-class AwsBaseHook(BaseHook):
+class AwsBaseHook(BaseHook, DiscoverableHook):
Review comment:
OK. I am good with that solves another problem where I had to remove the protocol from providers.
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537083714
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -303,7 +303,7 @@ def web_identity_token_loader():
return web_identity_token_loader
-class AwsBaseHook(BaseHook):
+class AwsBaseHook(BaseHook, DiscoverableHook):
Review comment:
I simply left the Protocol as a class that is not used - purely documentation and capturing the types. There is no need for any of the Hooks to derive from 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 a change in pull request #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537774829
##########
File path: airflow/www/forms.py
##########
@@ -230,14 +236,14 @@ class ConnectionForm(DynamicForm):
port = IntegerField(lazy_gettext('Port'), validators=[Optional()], widget=BS3TextFieldWidget())
extra = TextAreaField(lazy_gettext('Extra'), widget=BS3TextAreaFieldWidget())
- # You should add connection form fields in the hooks via get_connection_form_widgets() method
- # See for example airflow/providers/jdbc/hooks/jdbc.py
- # It is esed to customized the form, the forms elements get rendered
- # and results are stored in the extra field as json. All of these
- # need to be prefixed with extra__ and then the conn_type ___ as in
- # extra__{conn_type}__name. You can also hide form elements and rename
- # others from the connection_form.js file
+ # Used to store a dictionary of field customizations used to dynamically change available
+ # fields in ConnectionForm based on type of connection chosen
+ field_parameters = HiddenField(
+ lazy_gettext('field_parameters'), default=json.dumps(_get_field_customizations())
+ )
Review comment:
I could not make it works (rendering scripts in submition forms basically does not work), but I found a much better way. Instead of modifying the Model, I modified the View and FormWidget. I learned a bit how FAB works internally and it turned out that form widget is a better (and more natural) way to pass the field behaviour.
That actually led me to a code where ConnectionForm does not have any customizations, and all is done in the view part And it also let me to better place where to initialze the providers and I am doing it now after plugin initialization.
I decided to merge those two PRs, they make much more sense together than separately.
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537087239
##########
File path: airflow/www/static/js/connection_form.js
##########
@@ -87,7 +34,7 @@ $(document).ready(function () {
$(this).text($(this).attr("orig_text"));
});
$(".form-control").each(function(){$(this).attr('placeholder', '')});
-
+ let config = JSON.parse($("#field_parameters").text())
Review comment:
Right :facepalm: `compile_assets`
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537082531
##########
File path: airflow/providers/cloudant/hooks/cloudant.py
##########
@@ -16,13 +16,16 @@
# specific language governing permissions and limitations
# under the License.
"""Hook for Cloudant"""
+from typing import Dict
+
from cloudant import cloudant
+from wtforms import Field
Review comment:
I will just skip the 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] ashb commented on a change in pull request #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537130298
##########
File path: airflow/www/static/js/connection_form.js
##########
@@ -87,7 +34,7 @@ $(document).ready(function () {
$(this).text($(this).attr("orig_text"));
});
$(".form-control").each(function(){$(this).attr('placeholder', '')});
-
+ let config = JSON.parse($("#field_parameters").text())
Review comment:
Oh, no I'm wrong. When running in dev mode it still fingerprints, so the file name changes when the input does, meaning that the webserver needs to be restarted.
----------------------------------------------------------------
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] ashb commented on a change in pull request #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537057669
##########
File path: airflow/hooks/base_hook.py
##########
@@ -18,14 +18,82 @@
"""Base class for all hooks"""
import logging
import warnings
-from typing import Any, List
+from typing import Any, Dict, List
+
+from wtforms import Field
from airflow.models.connection import Connection
+from airflow.typing_compat import Protocol
from airflow.utils.log.logging_mixin import LoggingMixin
log = logging.getLogger(__name__)
+class DiscoverableHook(Protocol):
+ """
+ This protocol should be implemented by Hook classes that want to be discovered by ProvidersManager
+
+
+ The conn_name_attr, default_conn_name, conn_type should be implemented by those
+ Hooks that want to be automatically mapped from the connection_type -> Hook when get_hook method
+ is called with connection_type.
+
+ Additional - hook_name should be set when you want the hook to have a custom name in the UI selection
Review comment:
```suggestion
Additionally hook_name should be set when you want the hook to have a custom name in the UI selection
```
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537085289
##########
File path: airflow/providers/docker/hooks/docker.py
##########
@@ -39,6 +40,20 @@ class DockerHook(BaseHook, LoggingMixin):
conn_type = 'docker'
hook_name = 'Docker Registry'
+ @staticmethod
+ def get_custom_fields() -> Dict:
+ return {
+ "hidden_fields": ['port', 'schema'],
Review comment:
Ups. Good eye. That was a mistake in the original js:
```
docker: {
hidden_fields: ['port', 'schema'],
relabeling: {
'host': 'Registry URL',
'login': 'Username',
},
},
```
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537082993
##########
File path: airflow/providers/cloudant/hooks/cloudant.py
##########
@@ -37,6 +40,17 @@ class CloudantHook(BaseHook):
conn_type = 'cloudant'
hook_name = 'Cloudant'
+ @staticmethod
+ def get_custom_fields() -> Dict:
+ return {
+ "hidden_fields": ['port', 'extra'],
+ "relabeling": {'host': 'Account', 'login': 'Username (or API Key)', 'schema': 'Database'},
+ }
+
+ @staticmethod
+ def get_connection_form_widgets() -> Dict[str, Field]:
Review comment:
Yeah. I can remove it and then import is not even need.
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537086130
##########
File path: airflow/providers_manager.py
##########
@@ -244,7 +256,13 @@ def _add_hook(self, hook_class_name: str, provider_package: str) -> None:
# hierarchy and we add it only from the parent hook that provides those!
if 'get_connection_form_widgets' in hook_class.__dict__:
widgets = hook_class.get_connection_form_widgets()
- self._add_widgets(provider_package, hook_class, widgets)
+ if len(widgets.keys()) > 0:
+ self._add_widgets(provider_package, hook_class, widgets)
+ if 'get_custom_fields' in hook_class.__dict__:
+ customized_fields = hook_class.get_custom_fields()
+ if len(customized_fields.keys()) > 0:
Review comment:
Why not
----------------------------------------------------------------
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 closed pull request #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #12837:
URL: https://github.com/apache/airflow/pull/12837
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537079083
##########
File path: airflow/www/forms.py
##########
@@ -213,6 +215,10 @@ def _get_connection_types():
return _connection_types
+def _get_field_customizations() -> Dict[str, Dict]:
+ return ProvidersManager().field_customizations
Review comment:
True
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537079296
##########
File path: airflow/www/forms.py
##########
@@ -230,14 +236,14 @@ class ConnectionForm(DynamicForm):
port = IntegerField(lazy_gettext('Port'), validators=[Optional()], widget=BS3TextFieldWidget())
extra = TextAreaField(lazy_gettext('Extra'), widget=BS3TextAreaFieldWidget())
- # You should add connection form fields in the hooks via get_connection_form_widgets() method
- # See for example airflow/providers/jdbc/hooks/jdbc.py
- # It is esed to customized the form, the forms elements get rendered
- # and results are stored in the extra field as json. All of these
- # need to be prefixed with extra__ and then the conn_type ___ as in
- # extra__{conn_type}__name. You can also hide form elements and rename
- # others from the connection_form.js file
+ # Used to store a dictionary of field customizations used to dynamically change available
+ # fields in ConnectionForm based on type of connection chosen
+ field_parameters = HiddenField(
+ lazy_gettext('field_parameters'), default=json.dumps(_get_field_customizations())
+ )
Review comment:
Yep. Good one. Will try 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 a change in pull request #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537079989
##########
File path: airflow/www/static/js/connection_form.js
##########
@@ -87,7 +34,7 @@ $(document).ready(function () {
$(this).text($(this).attr("orig_text"));
});
$(".form-control").each(function(){$(this).attr('placeholder', '')});
-
+ let config = JSON.parse($("#field_parameters").text())
Review comment:
Strange. It worked.
----------------------------------------------------------------
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] ashb commented on a change in pull request #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537057669
##########
File path: airflow/hooks/base_hook.py
##########
@@ -18,14 +18,82 @@
"""Base class for all hooks"""
import logging
import warnings
-from typing import Any, List
+from typing import Any, Dict, List
+
+from wtforms import Field
from airflow.models.connection import Connection
+from airflow.typing_compat import Protocol
from airflow.utils.log.logging_mixin import LoggingMixin
log = logging.getLogger(__name__)
+class DiscoverableHook(Protocol):
+ """
+ This protocol should be implemented by Hook classes that want to be discovered by ProvidersManager
+
+
+ The conn_name_attr, default_conn_name, conn_type should be implemented by those
+ Hooks that want to be automatically mapped from the connection_type -> Hook when get_hook method
+ is called with connection_type.
+
+ Additional - hook_name should be set when you want the hook to have a custom name in the UI selection
Review comment:
```suggestion
Additionally hook_name should be set when you want the hook to have a custom name in the UI selection
```
##########
File path: airflow/www/static/js/connection_form.js
##########
@@ -87,7 +34,7 @@ $(document).ready(function () {
$(this).text($(this).attr("orig_text"));
});
$(".form-control").each(function(){$(this).attr('placeholder', '')});
-
+ let config = JSON.parse($("#field_parameters").text())
Review comment:
`.text()` on a hidden input returns nothing. You wanted `.val()`.
##########
File path: airflow/providers/cloudant/hooks/cloudant.py
##########
@@ -37,6 +40,17 @@ class CloudantHook(BaseHook):
conn_type = 'cloudant'
hook_name = 'Cloudant'
+ @staticmethod
+ def get_custom_fields() -> Dict:
+ return {
+ "hidden_fields": ['port', 'extra'],
+ "relabeling": {'host': 'Account', 'login': 'Username (or API Key)', 'schema': 'Database'},
+ }
+
+ @staticmethod
+ def get_connection_form_widgets() -> Dict[str, Field]:
Review comment:
So in cncf.kube.hooks.kube you don't implement this method, but here you do.
Do we _need_ to have both methods? Lets be consistent, and either remove this here, or add it so that when we have one we have both.
##########
File path: airflow/providers/apache/spark/hooks/spark_submit.py
##########
@@ -109,6 +111,17 @@ class SparkSubmitHook(BaseHook, LoggingMixin):
conn_type = 'spark'
hook_name = 'Spark'
+ @staticmethod
+ def get_connection_form_widgets() -> Dict[str, Field]:
+ return {}
+
+ @staticmethod
+ def get_custom_fields() -> Dict:
Review comment:
How about `get_ui_field_behaviour` as a name?
`custom_fields` sort of reads like this could return WTForms.Field instances, but that is not what we are doing here.
##########
File path: airflow/www/forms.py
##########
@@ -213,6 +215,10 @@ def _get_connection_types():
return _connection_types
+def _get_field_customizations() -> Dict[str, Dict]:
+ return ProvidersManager().field_customizations
Review comment:
Single use one line function -- why not "inline" it direct?
##########
File path: airflow/www/forms.py
##########
@@ -230,14 +236,14 @@ class ConnectionForm(DynamicForm):
port = IntegerField(lazy_gettext('Port'), validators=[Optional()], widget=BS3TextFieldWidget())
extra = TextAreaField(lazy_gettext('Extra'), widget=BS3TextAreaFieldWidget())
- # You should add connection form fields in the hooks via get_connection_form_widgets() method
- # See for example airflow/providers/jdbc/hooks/jdbc.py
- # It is esed to customized the form, the forms elements get rendered
- # and results are stored in the extra field as json. All of these
- # need to be prefixed with extra__ and then the conn_type ___ as in
- # extra__{conn_type}__name. You can also hide form elements and rename
- # others from the connection_form.js file
+ # Used to store a dictionary of field customizations used to dynamically change available
+ # fields in ConnectionForm based on type of connection chosen
+ field_parameters = HiddenField(
+ lazy_gettext('field_parameters'), default=json.dumps(_get_field_customizations())
+ )
Review comment:
Making this a field means this gets submitted back to the server which isn't needed.
Please find another way to get this in to the front end.
##########
File path: airflow/hooks/base_hook.py
##########
@@ -18,14 +18,82 @@
"""Base class for all hooks"""
import logging
import warnings
-from typing import Any, List
+from typing import Any, Dict, List
+
+from wtforms import Field
from airflow.models.connection import Connection
+from airflow.typing_compat import Protocol
from airflow.utils.log.logging_mixin import LoggingMixin
log = logging.getLogger(__name__)
+class DiscoverableHook(Protocol):
+ """
+ This protocol should be implemented by Hook classes that want to be discovered by ProvidersManager
+
+
+ The conn_name_attr, default_conn_name, conn_type should be implemented by those
+ Hooks that want to be automatically mapped from the connection_type -> Hook when get_hook method
+ is called with connection_type.
+
+ Additional - hook_name should be set when you want the hook to have a custom name in the UI selection
+ Name. If not specified, conn_name will be used.
+
+ The "get_custom_fields" and "get_connection_form_widgets" are optional - override them if you want
+ to customize the Connection Form screen. You can add extra widgets to parse your extra fields via the
+ get_connection_form_widgets method as well as hide or relabel the fields or pre-fill
+ them with placeholders via get_custom_fields method.
+
+ Note that the "get_custom_fields" and "get_connection_form_widgets" need to be set by each class in the
+ class hierarchy in order to apply widget customizations.
Review comment:
Add;
```rst
For example, even if you want to use the fields from your parent class, you must explicitly have a method on *your* class:
.. code-block:: python
@classmethod
def get_custom_fields(cls):
return super().get_custom_fields()
```
##########
File path: airflow/providers/cloudant/hooks/cloudant.py
##########
@@ -16,13 +16,16 @@
# specific language governing permissions and limitations
# under the License.
"""Hook for Cloudant"""
+from typing import Dict
+
from cloudant import cloudant
+from wtforms import Field
Review comment:
I really don't like importing just for types. _Especially_ as we return `{}`
Please use `typing.TYPE_CHECKING` here.
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -303,7 +303,7 @@ def web_identity_token_loader():
return web_identity_token_loader
-class AwsBaseHook(BaseHook):
+class AwsBaseHook(BaseHook, DiscoverableHook):
Review comment:
Three things:
1. Protocols are not _normally_ explicit base classes -- they are more normally used as "duck-typing" only. Check out `airflow.models.crypto.FernetProtocol` -- it is not a subclass, but just a type.
2. This class _doesn't_ implement the discoverable protocol, it doesn't have `get_connection_form_widgets` or `get_custom_fields` methods.
Additionally a result of the sublclassing, the type is wrong.
```python console
>>> from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
>>> AwsBaseHook.get_custom_fields
<function airflow.hooks.base_hook.DiscoverableHook.get_custom_fields() -> Dict>
>>> AwsBaseHook.get_custom_fields()
>>> type(AwsBaseHook.get_custom_fields())
NoneType
```
(Yes, the view wouldn't use this because of the checks I know.)
My preferred solution to this would be to never inherit from the protocol.
##########
File path: airflow/providers/docker/hooks/docker.py
##########
@@ -39,6 +40,20 @@ class DockerHook(BaseHook, LoggingMixin):
conn_type = 'docker'
hook_name = 'Docker Registry'
+ @staticmethod
+ def get_custom_fields() -> Dict:
+ return {
+ "hidden_fields": ['port', 'schema'],
Review comment:
But we use port:
```
if conn.port:
self.__registry = f"{conn.host}:{conn.port}"
```
##########
File path: airflow/providers_manager.py
##########
@@ -289,6 +307,28 @@ def _add_widgets(self, package_name: str, hook_class: type, widgets: Dict[str, F
hook_class.__name__, package_name, field
)
+ def _add_customized_fields(self, package_name: str, hook_class: type, customized_fields: Dict):
+ try:
+ connection_type = getattr(hook_class, "conn_type")
+ self._customized_form_fields_schema_validator.validate(customized_fields)
+ if connection_type in self._field_customizations:
+ log.warning(
+ "The connection_type %s from package %s and class %s has already been added "
+ "by another provider. Ignoring it.",
+ connection_type,
+ package_name,
+ hook_class.__name__,
+ )
+ return
+ self._field_customizations[connection_type] = customized_fields
+ except Exception as e: # noqa pylint: disable=broad-except
+ log.warning(
+ "Error when loading customized fields from package '%s' " "hook class '%s': %s",
Review comment:
```suggestion
"Error when loading customized fields from package '%s' hook class '%s': %s",
```
##########
File path: airflow/providers_manager.py
##########
@@ -325,12 +365,17 @@ def hooks(self) -> Dict[str, HookInfo]:
"""Returns dictionary of connection_type-to-hook mapping"""
return self._hooks_dict
- @property
- def extra_links_class_names(self):
- """Returns set of extra link class names."""
- return sorted(list(self._extra_link_class_name_set))
-
@property
def connection_form_widgets(self) -> Dict[str, ConnectionFormWidgetInfo]:
"""Returns widgets for connection forms."""
return self._connection_form_widgets
+
+ @property
+ def field_customizations(self) -> Dict[str, Dict]:
+ """Returns dictionary with field customizations for connection types."""
+ return self._field_customizations
+
+ @property
+ def extra_links_class_names(self):
+ """Returns set of extra link class names."""
+ return sorted(list(self._extra_link_class_name_set))
Review comment:
```suggestion
return sorted(self._extra_link_class_name_set)
```
##########
File path: airflow/providers_manager.py
##########
@@ -244,7 +256,13 @@ def _add_hook(self, hook_class_name: str, provider_package: str) -> None:
# hierarchy and we add it only from the parent hook that provides those!
if 'get_connection_form_widgets' in hook_class.__dict__:
widgets = hook_class.get_connection_form_widgets()
- self._add_widgets(provider_package, hook_class, widgets)
+ if len(widgets.keys()) > 0:
+ self._add_widgets(provider_package, hook_class, widgets)
+ if 'get_custom_fields' in hook_class.__dict__:
+ customized_fields = hook_class.get_custom_fields()
+ if len(customized_fields.keys()) > 0:
Review comment:
```suggestion
if customized_fields:
```
##########
File path: airflow/www/forms.py
##########
@@ -230,14 +236,14 @@ class ConnectionForm(DynamicForm):
port = IntegerField(lazy_gettext('Port'), validators=[Optional()], widget=BS3TextFieldWidget())
extra = TextAreaField(lazy_gettext('Extra'), widget=BS3TextAreaFieldWidget())
- # You should add connection form fields in the hooks via get_connection_form_widgets() method
- # See for example airflow/providers/jdbc/hooks/jdbc.py
- # It is esed to customized the form, the forms elements get rendered
- # and results are stored in the extra field as json. All of these
- # need to be prefixed with extra__ and then the conn_type ___ as in
- # extra__{conn_type}__name. You can also hide form elements and rename
- # others from the connection_form.js file
+ # Used to store a dictionary of field customizations used to dynamically change available
+ # fields in ConnectionForm based on type of connection chosen
+ field_parameters = HiddenField(
+ lazy_gettext('field_parameters'), default=json.dumps(_get_field_customizations())
+ )
Review comment:
I think you can do this with a custom widget:
```python
class provider_field_widget(field, **kwargs):
return Markup('<script id="{}" type="text/json">{}</script>').format(
field.name,
json.dumps(ProvidersManager().field_customizations)
)
```
And then:
```suggestion
field_parameters = HiddenField(
lazy_gettext('field_parameters'), widget=provider_field_widget
)
```
(And `$("#field_parameters").text()` would continue to work)
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#issuecomment-740134620
Hey @ashb - It turned out that i can do it much nicer and that it makes sense to join these two PRs and implementing both - custom fields and fiedl_behaviour in single PR. All problems should be sovled in #12558 so closing this one.
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#issuecomment-739315616
I've found rather simple and nice way of passing the discovered "hidden/renamed fields" to the javascript form. I do it via Hidden Field in FAB. This Is really simple and straightforward comparing to any kind of AJAX approach (which we discussed with @mik-laj before). Everything is nicely localised in one place and connection form has the dictionary available immediately without running any ajax call.
This is the last part of the provider customization. it is based on #12837 so look only at the last commit please.
ONce this is merged, I will add "How to write provider" and will close this topic.
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#issuecomment-739557209
[The Workflow run](https://github.com/apache/airflow/actions/runs/404439868) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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] ashb commented on a change in pull request #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537129887
##########
File path: airflow/www/static/js/connection_form.js
##########
@@ -87,7 +34,7 @@ $(document).ready(function () {
$(this).text($(this).attr("orig_text"));
});
$(".form-control").each(function(){$(this).attr('placeholder', '')});
-
+ let config = JSON.parse($("#field_parameters").text())
Review comment:
It doesn't help with the "rebuild on deman" but if you run `yarn --cwd airflow/www dev` it will run a compiler that watches for changes and automatically rebuilds assets. I _think_ it also doesn't fingerprint the assets, so the manifest at the start is valid even when content changes.
----------------------------------------------------------------
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] ashb commented on a change in pull request #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537129221
##########
File path: airflow/providers_manager.py
##########
@@ -209,22 +279,63 @@ def _add_hook(self, hook_class_name, provider_package) -> None:
e,
)
return
- conn_type = getattr(hook_class, 'conn_type')
- if not conn_type:
- log.warning(
- "The hook_class '%s' is missing connection_type attribute and cannot be registered",
- hook_class,
- )
+
+ conn_type: str = self._get_attr(hook_class, 'conn_type')
+ connection_id_attribute_name: str = self._get_attr(hook_class, 'conn_name_attr')
+ hook_name: str = self._get_attr(hook_class, 'hook_name')
+
+ if not conn_type or not connection_id_attribute_name or not hook_name:
return
- connection_id_attribute_name = getattr(hook_class, 'conn_name_attr')
- if not connection_id_attribute_name:
- log.warning(
- "The hook_class '%s' is missing conn_name_attr attribute and cannot be registered",
- hook_class,
+
+ self._hooks_dict[conn_type] = HookInfo(
+ hook_class_name,
+ connection_id_attribute_name,
+ provider_package,
+ hook_name,
+ )
+
+ def _add_widgets(self, package_name: str, hook_class: type, widgets: Dict[str, Field]):
+ for field_name, field in widgets.items():
+ if not field_name.startswith("extra__"):
+ log.warning(
+ "The field %s from class %s does not start with 'extra__'. Ignoring it.",
+ field_name,
+ hook_class.__name__,
+ )
+ continue
+ if field_name in self._connection_form_widgets:
+ log.warning(
+ "The field %s from class %s has already been added by another provider. Ignoring it.",
+ field_name,
+ hook_class.__name__,
+ )
+ # In case of inherited hooks this might be happening several times
+ continue
+ self._connection_form_widgets[field_name] = ConnectionFormWidgetInfo(
+ hook_class.__name__, package_name, field
)
- return
- self._hooks_dict[conn_type] = (hook_class_name, connection_id_attribute_name)
+ def _add_customized_fields(self, package_name: str, hook_class: type, customized_fields: Dict):
Review comment:
```suggestion
def _add_field_behaviours(self, package_name: str, hook_class: type, customized_fields: Dict):
```
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#discussion_r537086248
##########
File path: airflow/providers_manager.py
##########
@@ -289,6 +307,28 @@ def _add_widgets(self, package_name: str, hook_class: type, widgets: Dict[str, F
hook_class.__name__, package_name, field
)
+ def _add_customized_fields(self, package_name: str, hook_class: type, customized_fields: Dict):
+ try:
+ connection_type = getattr(hook_class, "conn_type")
+ self._customized_form_fields_schema_validator.validate(customized_fields)
+ if connection_type in self._field_customizations:
+ log.warning(
+ "The connection_type %s from package %s and class %s has already been added "
+ "by another provider. Ignoring it.",
+ connection_type,
+ package_name,
+ hook_class.__name__,
+ )
+ return
+ self._field_customizations[connection_type] = customized_fields
+ except Exception as e: # noqa pylint: disable=broad-except
+ log.warning(
+ "Error when loading customized fields from package '%s' " "hook class '%s': %s",
Review comment:
ah black joined 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] github-actions[bot] commented on pull request #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#issuecomment-739600275
[The Workflow run](https://github.com/apache/airflow/actions/runs/404723571) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#issuecomment-739325769
[The Workflow run](https://github.com/apache/airflow/actions/runs/402844832) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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 #12837: Customizable javascript configuration in connnection form
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12837:
URL: https://github.com/apache/airflow/pull/12837#issuecomment-739557238
[The Workflow run](https://github.com/apache/airflow/actions/runs/404440137) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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