shahar1 commented on code in PR #47034:
URL: https://github.com/apache/airflow/pull/47034#discussion_r1968282907
##########
task_sdk/src/airflow/sdk/definitions/asset/decorators.py:
##########
@@ -25,27 +25,32 @@
from airflow.providers.standard.operators.python import PythonOperator
from airflow.sdk.definitions.asset import Asset, AssetNameRef, AssetRef,
BaseAsset
+# ✅ Import the correct DAG class from SDK
+from airflow.sdk.dag import SdkDAG # ✅ Updated import
Review Comment:
1. While emojis like ✅ are valid UTF-8 characters - it's quite unusual to
put them within comments.
2. In general, there's no need to comment every single line of code - unless
there's a good reason to do so.
##########
task_sdk/src/airflow/sdk/definitions/asset/decorators.py:
##########
@@ -25,27 +25,32 @@
from airflow.providers.standard.operators.python import PythonOperator
Review Comment:
It's entirely unclear why the modifications for this file are needed.
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -91,63 +92,30 @@ class SnowflakeHook(DbApiHook):
supports_autocommit = True
_test_connection_sql = "select 1"
- @classmethod
- def get_connection_form_widgets(cls) -> dict[str, Any]:
- """Return connection widgets to add to connection form."""
- from flask_appbuilder.fieldwidgets import (
- BS3PasswordFieldWidget,
- BS3TextFieldWidget,
- )
- from flask_babel import lazy_gettext
- from wtforms import BooleanField, PasswordField, StringField
-
- return {
- "account": StringField(lazy_gettext("Account"),
widget=BS3TextFieldWidget()),
- "warehouse": StringField(lazy_gettext("Warehouse"),
widget=BS3TextFieldWidget()),
- "database": StringField(lazy_gettext("Database"),
widget=BS3TextFieldWidget()),
- "region": StringField(lazy_gettext("Region"),
widget=BS3TextFieldWidget()),
- "role": StringField(lazy_gettext("Role"),
widget=BS3TextFieldWidget()),
- "private_key_file": StringField(lazy_gettext("Private key
(Path)"), widget=BS3TextFieldWidget()),
- "private_key_content": PasswordField(
- lazy_gettext("Private key (Text)"),
widget=BS3PasswordFieldWidget()
- ),
- "insecure_mode": BooleanField(
- label=lazy_gettext("Insecure mode"), description="Turns off
OCSP certificate checks"
- ),
- }
-
- @classmethod
- def get_ui_field_behaviour(cls) -> dict[str, Any]:
- """Return custom field behaviour."""
- import json
-
- return {
- "hidden_fields": ["port", "host"],
- "relabeling": {},
- "placeholders": {
- "extra": json.dumps(
- {
- "authenticator": "snowflake oauth",
- "private_key_file": "private key",
- "session_parameters": "session parameters",
- "client_request_mfa_token": "client request mfa token",
- "client_store_temporary_credential": "client store
temporary credential (externalbrowser mode)",
- },
- indent=1,
- ),
- "schema": "snowflake schema",
- "login": "snowflake username",
- "password": "snowflake password",
- "account": "snowflake account name",
- "warehouse": "snowflake warehouse name",
- "database": "snowflake db name",
- "region": "snowflake hosted region",
- "role": "snowflake role",
- "private_key_file": "Path of snowflake private key (PEM
Format)",
- "private_key_content": "Content to snowflake private key (PEM
format)",
- "insecure_mode": "insecure mode",
- },
- }
+ @classmethod
+def get_connection_form_widgets(cls) -> dict[str, Any]:
+ from wtforms import StringField
+ from flask_appbuilder.fieldwidgets import BS3TextAreaFieldWidget
+ from flask_babel import lazy_gettext
+
+ return {
+ "private_key_content": StringField(
+ lazy_gettext("Private key (Text)"), widget=BS3TextAreaFieldWidget()
+ ),
+ # ... other fields ...
+ }
+
+@classmethod
+def get_ui_field_behaviour(cls) -> dict[str, Any]:
+ return {
+ "hidden_fields": ["extra"],
+ "relabeling": {},
+ "placeholders": {
+ "private_key_content": "Paste your private key here",
+ # ... other placeholders ...
+ },
+ "sensitive_fields": ["private_key_content"], # Mark as sensitive
+ }
Review Comment:
Same question - why have the other fields been removed?
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -91,63 +92,30 @@ class SnowflakeHook(DbApiHook):
supports_autocommit = True
_test_connection_sql = "select 1"
- @classmethod
- def get_connection_form_widgets(cls) -> dict[str, Any]:
- """Return connection widgets to add to connection form."""
- from flask_appbuilder.fieldwidgets import (
- BS3PasswordFieldWidget,
- BS3TextFieldWidget,
- )
- from flask_babel import lazy_gettext
- from wtforms import BooleanField, PasswordField, StringField
-
- return {
- "account": StringField(lazy_gettext("Account"),
widget=BS3TextFieldWidget()),
- "warehouse": StringField(lazy_gettext("Warehouse"),
widget=BS3TextFieldWidget()),
- "database": StringField(lazy_gettext("Database"),
widget=BS3TextFieldWidget()),
- "region": StringField(lazy_gettext("Region"),
widget=BS3TextFieldWidget()),
- "role": StringField(lazy_gettext("Role"),
widget=BS3TextFieldWidget()),
- "private_key_file": StringField(lazy_gettext("Private key
(Path)"), widget=BS3TextFieldWidget()),
- "private_key_content": PasswordField(
- lazy_gettext("Private key (Text)"),
widget=BS3PasswordFieldWidget()
- ),
- "insecure_mode": BooleanField(
- label=lazy_gettext("Insecure mode"), description="Turns off
OCSP certificate checks"
- ),
- }
-
- @classmethod
- def get_ui_field_behaviour(cls) -> dict[str, Any]:
- """Return custom field behaviour."""
- import json
-
- return {
- "hidden_fields": ["port", "host"],
- "relabeling": {},
- "placeholders": {
- "extra": json.dumps(
- {
- "authenticator": "snowflake oauth",
- "private_key_file": "private key",
- "session_parameters": "session parameters",
- "client_request_mfa_token": "client request mfa token",
- "client_store_temporary_credential": "client store
temporary credential (externalbrowser mode)",
- },
- indent=1,
- ),
- "schema": "snowflake schema",
- "login": "snowflake username",
- "password": "snowflake password",
- "account": "snowflake account name",
- "warehouse": "snowflake warehouse name",
- "database": "snowflake db name",
- "region": "snowflake hosted region",
- "role": "snowflake role",
- "private_key_file": "Path of snowflake private key (PEM
Format)",
- "private_key_content": "Content to snowflake private key (PEM
format)",
- "insecure_mode": "insecure mode",
- },
- }
+ @classmethod
+def get_connection_form_widgets(cls) -> dict[str, Any]:
+ from wtforms import StringField
+ from flask_appbuilder.fieldwidgets import BS3TextAreaFieldWidget
+ from flask_babel import lazy_gettext
+
+ return {
+ "private_key_content": StringField(
+ lazy_gettext("Private key (Text)"), widget=BS3TextAreaFieldWidget()
+ ),
+ # ... other fields ...
Review Comment:
Why have the other fields been removed?
##########
task_sdk/src/airflow/sdk/dag.py:
##########
@@ -0,0 +1,9 @@
+from airflow.models.dag import DAG
+
+class SdkDAG(DAG):
+ """
+ Custom SDK-based DAG class for improved modularity.
+ Inherits from Airflow's DAG class.
+ """
+ def __init__(self, dag_id, **kwargs):
+ super().__init__(dag_id=dag_id, **kwargs)
Review Comment:
1. Adding a custom class that only inherits from another class while only
changing its constructor doesn't really add modularity.
2. It's not clear why this addition is needed.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
