[GitHub] [airflow] ferruzzi commented on a diff in pull request #24239: AIP-47 - Migrate redshift DAGs to new design #22438

[GitBox <gi...@apache.org>]

ferruzzi commented on code in PR #24239:
URL: https://github.com/apache/airflow/pull/24239#discussion_r890455678


##########
tests/system/providers/amazon/aws/redshift/__init__.py:
##########
@@ -0,0 +1,17 @@
+#

Review Comment:
   So far, the plan has been that all tests should live at the 'aws' level, not in a subdirectory/module, just like everywhere else (aws/operators, aws/hooks, etc).  Is there a particular reason you went this route?



##########
tests/system/providers/amazon/aws/redshift/example_redshift_cluster.py:
##########
@@ -28,7 +28,11 @@
     RedshiftResumeClusterOperator,
 )
 from airflow.providers.amazon.aws.sensors.redshift_cluster import RedshiftClusterSensor
+from airflow.utils.trigger_rule import TriggerRule
+from tests.system.providers.amazon.aws.utils import set_env_id
 
+ENV_ID = set_env_id()
+DAG_ID = 'example_redshift_cluster'

Review Comment:
   I can't comment below, but this should be used in the "with DAG" block:
   
   with DAG(
       dag_id=DAG_ID,
       ....
       ) as dag:



##########
tests/system/providers/amazon/aws/redshift/example_redshift_to_s3.py:
##########
@@ -20,13 +20,16 @@
 
 from airflow import DAG
 from airflow.providers.amazon.aws.transfers.redshift_to_s3 import RedshiftToS3Operator
+from tests.system.providers.amazon.aws.utils import set_env_id
 
+ENV_ID = set_env_id()
+DAG_ID = 'example_redshift_to_s3'
 S3_BUCKET_NAME = getenv("S3_BUCKET_NAME", "s3_bucket_name")
 S3_KEY = getenv("S3_KEY", "s3_key")
 REDSHIFT_TABLE = getenv("REDSHIFT_TABLE", "redshift_table")
 
 with DAG(
-    dag_id="example_redshift_to_s3",
+    dag_id=DAG_ID,

Review Comment:
   You did it in this one :stuck_out_tongue: 



##########
tests/system/providers/amazon/aws/redshift/example_redshift_sql.py:
##########
@@ -21,6 +21,11 @@
 from airflow import DAG
 from airflow.models.baseoperator import chain
 from airflow.providers.amazon.aws.operators.redshift_sql import RedshiftSQLOperator
+from airflow.utils.trigger_rule import TriggerRule
+from tests.system.providers.amazon.aws.utils import set_env_id
+
+ENV_ID = set_env_id()
+DAG_ID = 'example_redshift_sql'

Review Comment:
   I can't comment below, but this should be used in the "with DAG" block:
   
   with DAG(
       dag_id=DAG_ID,
       ....
       ) as dag:



##########
tests/system/providers/amazon/aws/redshift/example_redshift_data_execute_sql.py:
##########
@@ -22,7 +22,11 @@
 from airflow.decorators import task
 from airflow.providers.amazon.aws.hooks.redshift_data import RedshiftDataHook
 from airflow.providers.amazon.aws.operators.redshift_data import RedshiftDataOperator
+from airflow.utils.trigger_rule import TriggerRule
+from tests.system.providers.amazon.aws.utils import set_env_id
 
+ENV_ID = set_env_id()
+DAG_ID = 'example_redshift_cluster'

Review Comment:
   I can't comment below, but this should be used in the "with DAG" block:
   
   with DAG(
       dag_id=DAG_ID,
       ....
       ) as dag:



##########
tests/system/providers/amazon/aws/redshift/example_redshift_data_execute_sql.py:
##########
@@ -39,7 +43,7 @@
 POLL_INTERVAL = 10
 
 
-@task(task_id="output_results")
+@task(task_id="output_results", trigger_rule=TriggerRule.ALL_DONE)

Review Comment:
   Nitpick:  Is there a particular reason to change the name like that?   If you name the method 'output_results' then you don't need to explicitly set the task_id, or drop the task_id assignment and it generates one using the method 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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org