Repository: TobikoData/sqlmesh
Pull request #2747 (2024-06-06 16:47:56)

Feat: improve cicd comment message format
Old format:
<img width="368" alt="Screenshot 2024-06-06 at 9 47 25 AM" src="https://github.com/TobikoData/sqlmesh/assets/6326532/33d7ed76-0784-47c6-8db4-409c9790a0ce">

New format:
<img width="491" alt="Screenshot 2024-06-06 at 9 54 26 AM" src="https://github.com/TobikoData/sqlmesh/assets/6326532/598a482f-7cdf-4d0d-9f36-63930975b793">



The bullet about issuing a `deploy` command will only show up if the project has the deploy command configured.



Patch:
diff --git a/sqlmesh/core/model/definition.py b/sqlmesh/core/model/definition.py
index bffdd89e..aca1bcf7 100644
--- a/sqlmesh/core/model/definition.py
+++ b/sqlmesh/core/model/definition.py
@@ -1983,10 +1983,10 @@ def _python_env(
                     elif name in variables:
                         used_variables.add(name)
                 elif (
-                    isinstance(macro_func_or_var, exp.Identifier) and "@" in macro_func_or_var.this
-                ):
+                    isinstance(macro_func_or_var, (exp.Identifier, d.MacroStrReplace, d.MacroSQL))
+                ) and "@" in macro_func_or_var.name:
                     for _, identifier, braced_identifier, _ in MacroStrTemplate.pattern.findall(
-                        macro_func_or_var.this
+                        macro_func_or_var.name
                     ):
                         var_name = braced_identifier or identifier
                         if var_name in variables:
diff --git a/sqlmesh/integrations/github/cicd/controller.py b/sqlmesh/integrations/github/cicd/controller.py
index e629c061..cd868f6e 100644
--- a/sqlmesh/integrations/github/cicd/controller.py
+++ b/sqlmesh/integrations/github/cicd/controller.py
@@ -279,7 +279,7 @@ class GithubEvent:
 
 
 class GithubController:
-    BOT_HEADER_MSG = "**SQLMesh Bot Info**"
+    BOT_HEADER_MSG = ":robot: **SQLMesh Bot Info** :robot:"
     MAX_BYTE_LENGTH = 65535
 
     def __init__(
@@ -565,7 +565,7 @@ class GithubController:
                 "Please check PR and resolve any issues."
             )
         plan_summary = f"""<details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 {self.get_plan_summary(self.prod_plan)}
 </details>
@@ -814,9 +814,16 @@ class GithubController:
                     table_header = h("thead", [h("tr", row) for row in header_rows])
                     table_body = h("tbody", [h("tr", row) for row in body_rows])
                     summary = str(h("table", [table_header, table_body]))
+                vde_title = (
+                    "- :eyes: To **review** this PR's changes, use virtual data environment:"
+                )
+                comment_value = f"{vde_title}\n  - `{self.pr_environment_name}`"
+                if self.bot_config.enable_deploy_command:
+                    comment_value += "\n- :arrow_forward: To apply this PR's plan to prod, comment:\n  - `/deploy`"
+                dedup_regex = vde_title.replace("*", r"\*") + r".*"
                 updated_comment, _ = self.update_sqlmesh_comment_info(
-                    value=f"- {check_title}",
-                    dedup_regex=rf"- {check_title_static}.*",
+                    value=comment_value,
+                    dedup_regex=dedup_regex,
                 )
                 if updated_comment:
                     self._append_output("created_pr_environment", "true")
diff --git a/sqlmesh/schedulers/airflow/dag_generator.py b/sqlmesh/schedulers/airflow/dag_generator.py
index c06d9381..9f00c017 100644
--- a/sqlmesh/schedulers/airflow/dag_generator.py
+++ b/sqlmesh/schedulers/airflow/dag_generator.py
@@ -257,6 +257,7 @@ class SnapshotDagGenerator:
 
             finalize_task = self._create_finalize_task(plan_dag_spec.environment)
             before_finalize_task >> finalize_task
+            finalize_task >> end_task
 
             on_plan_apply_end_task = PythonOperator(
                 task_id="on_plan_apply_end",
@@ -266,9 +267,7 @@ class SnapshotDagGenerator:
             )
             finalize_task >> on_plan_apply_end_task
 
-            self._add_notification_target_tasks(
-                plan_dag_spec, start_task, end_task, on_plan_apply_end_task
-            )
+            self._add_notification_target_tasks(plan_dag_spec, start_task, finalize_task)
             return dag
 
     def _add_notification_target_tasks(
@@ -276,9 +275,7 @@ class SnapshotDagGenerator:
         request: common.PlanDagSpec,
         start_task: BaseOperator,
         end_task: BaseOperator,
-        previous_end_task: BaseOperator,
     ) -> None:
-        has_success_or_failed_notification = False
         for notification_target in request.notification_targets:
             notification_operator_provider = NOTIFICATION_TARGET_TO_OPERATOR_PROVIDER.get(
                 type(notification_target)
@@ -297,15 +294,9 @@ class SnapshotDagGenerator:
             if plan_start_notification_task:
                 start_task >> plan_start_notification_task
             if plan_success_notification_task:
-                has_success_or_failed_notification = True
-                previous_end_task >> plan_success_notification_task
-                plan_success_notification_task >> end_task
+                end_task >> plan_success_notification_task
             if plan_failed_notification_task:
-                has_success_or_failed_notification = True
-                previous_end_task >> plan_failed_notification_task
-                plan_failed_notification_task >> end_task
-        if not has_success_or_failed_notification:
-            previous_end_task >> end_task
+                end_task >> plan_failed_notification_task
 
     def _create_creation_tasks(
         self,


Test patch:
diff --git a/tests/core/test_model.py b/tests/core/test_model.py
index 4e655cfc..76baccc3 100644
--- a/tests/core/test_model.py
+++ b/tests/core/test_model.py
@@ -3887,20 +3887,34 @@ def test_variables():
             kind FULL,
         );
 
-        SELECT @VAR('TEST_VAR_A') AS a, @VAR('test_var_b', 'default_value') AS b, @VAR('test_var_c') AS c, @TEST_MACRO_VAR() AS d;
+        SELECT
+          @VAR('TEST_VAR_A') AS a,
+          @VAR('test_var_b', 'default_value') AS b,
+          @VAR('test_var_c') AS c,
+          @TEST_MACRO_VAR() AS d,
+          @'foo_@{test_var_e}' AS e,
+          @SQL(foo_@{test_var_f}) AS f,
+          'foo_@{test_var_unused}' AS g
     """,
         default_dialect="bigquery",
     )
 
     model = load_sql_based_model(
-        expressions, variables={"test_var_a": "test_value", "test_var_d": 1, "test_var_unused": 2}
+        expressions,
+        variables={
+            "test_var_a": "test_value",
+            "test_var_d": 1,
+            "test_var_e": 4,
+            "test_var_f": 5,
+            "test_var_unused": 2,
+        },
     )
     assert model.python_env[c.SQLMESH_VARS] == Executable.value(
-        {"test_var_a": "test_value", "test_var_d": 1}
+        {"test_var_a": "test_value", "test_var_d": 1, "test_var_e": 4, "test_var_f": 5}
     )
     assert (
         model.render_query().sql(dialect="bigquery")
-        == "SELECT 'test_value' AS `a`, 'default_value' AS `b`, NULL AS `c`, 11 AS `d`"
+        == "SELECT 'test_value' AS `a`, 'default_value' AS `b`, NULL AS `c`, 11 AS `d`, 'foo_4' AS `e`, `foo_5` AS `f`, 'foo_@{test_var_unused}' AS `g`"
     )
 
     with pytest.raises(ConfigError, match=r"Macro VAR requires at least one argument.*"):
diff --git a/tests/integrations/github/cicd/test_github_commands.py b/tests/integrations/github/cicd/test_github_commands.py
index 940fe32e..e529a6f8 100644
--- a/tests/integrations/github/cicd/test_github_commands.py
+++ b/tests/integrations/github/cicd/test_github_commands.py
@@ -138,10 +138,11 @@ def test_run_all_success_with_approvers_approved(
 
     assert len(created_comments) == 1
     assert created_comments[0].body.startswith(
-        """**SQLMesh Bot Info**
-- PR Virtual Data Environment: myoverride_2
+        """:robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `myoverride_2`
 <details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 **New environment `prod` will be created from `prod`**"""
     )
@@ -264,10 +265,11 @@ def test_run_all_success_with_approvers_approved_merge_delete(
 
     assert len(created_comments) == 1
     assert created_comments[0].body.startswith(
-        """**SQLMesh Bot Info**
-- PR Virtual Data Environment: hello_world_2
+        """:robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`
 <details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 **New environment `prod` will be created from `prod`**"""
     )
@@ -390,7 +392,9 @@ def test_run_all_missing_approval(
     assert len(created_comments) == 1
     assert (
         created_comments[0].body
-        == """**SQLMesh Bot Info**\n- PR Virtual Data Environment: hello_world_2"""
+        == """:robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`"""
     )
     with open(github_output_file, "r", encoding="utf-8") as f:
         output = f.read()
@@ -902,10 +906,11 @@ def test_prod_update_failure(
 
     assert len(created_comments) == 1
     assert created_comments[0].body.startswith(
-        """**SQLMesh Bot Info**
-- PR Virtual Data Environment: hello_world_2
+        """:robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`
 <details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 **New environment `prod` will be created from `prod`**"""
     )
@@ -1095,10 +1100,13 @@ def test_comment_command_deploy_prod(
 
     assert len(created_comments) == 1
     assert created_comments[0].body.startswith(
-        """**SQLMesh Bot Info**
-- PR Virtual Data Environment: hello_world_2
+        """:robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`
+- :arrow_forward: To apply this PR's plan to prod, comment:
+  - `/deploy`
 <details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 **New environment `prod` will be created from `prod`**"""
     )
diff --git a/tests/integrations/github/cicd/test_github_controller.py b/tests/integrations/github/cicd/test_github_controller.py
index 856d437e..1104ac28 100644
--- a/tests/integrations/github/cicd/test_github_controller.py
+++ b/tests/integrations/github/cicd/test_github_controller.py
@@ -317,8 +317,8 @@ update_sqlmesh_comment_info_params = [
         [],
         "test1",
         None,
-        "**SQLMesh Bot Info**\ntest1",
-        "**SQLMesh Bot Info**\ntest1",
+        ":robot: **SQLMesh Bot Info** :robot:\ntest1",
+        ":robot: **SQLMesh Bot Info** :robot:\ntest1",
     ),
     (
         "Existing comments that are not related, one is created and then updated with value",
@@ -328,43 +328,43 @@ update_sqlmesh_comment_info_params = [
         ],
         "test1",
         None,
-        "**SQLMesh Bot Info**\ntest1",
-        "**SQLMesh Bot Info**\ntest1",
+        ":robot: **SQLMesh Bot Info** :robot:\ntest1",
+        ":robot: **SQLMesh Bot Info** :robot:\ntest1",
     ),
     (
         "Existing bot comment, that is updated and create comment is not called",
         [
-            MockIssueComment(body="**SQLMesh Bot Info**\ntest2"),
+            MockIssueComment(body=":robot: **SQLMesh Bot Info** :robot:\ntest2"),
             MockIssueComment(body="test3"),
         ],
         "test1",
         None,
-        "**SQLMesh Bot Info**\ntest2\ntest1",
+        ":robot: **SQLMesh Bot Info** :robot:\ntest2\ntest1",
         None,
     ),
     (
         "Existing bot comment, that is not updated because of dedup_regex and create comment is not called",
         [
-            MockIssueComment(body="**SQLMesh Bot Info**\ntest2"),
+            MockIssueComment(body=":robot: **SQLMesh Bot Info** :robot:\ntest2"),
             MockIssueComment(body="test3"),
         ],
         "test1",
         "test2",
-        "**SQLMesh Bot Info**\ntest2",
+        ":robot: **SQLMesh Bot Info** :robot:\ntest2",
         None,
     ),
     (
         "Ensure comments are truncated if they are too long",
         [
-            MockIssueComment(body="**SQLMesh Bot Info**\ntest1"),
+            MockIssueComment(body=":robot: **SQLMesh Bot Info** :robot:\ntest1"),
         ],
         # Making sure that although we will be under the character limit of `65535` we will still truncate
         # because the byte size of this character is 3 and therefore we will be over the limit since it is based
         # on bytes on not characters (despite what the error message may say)
         "桜" * 65000,
         None,
-        # ((Max Byte Length) - (Length of "**SQLMesh Bot Info**\ntest1\n")) / (Length of "桜")
-        "**SQLMesh Bot Info**\ntest1\n" + ("桜" * int((65535 - 27) / 3)),
+        # ((Max Byte Length) - (Length of ":robot: **SQLMesh Bot Info** :robot:\ntest1\n")) / (Length of "桜")
+        ":robot: **SQLMesh Bot Info** :robot:\ntest1\n" + ("桜" * int((65535 - 43) / 3)),
         None,
     ),
 ]
diff --git a/tests/integrations/github/cicd/test_integration.py b/tests/integrations/github/cicd/test_integration.py
index c068715d..a6bae22f 100644
--- a/tests/integrations/github/cicd/test_integration.py
+++ b/tests/integrations/github/cicd/test_integration.py
@@ -227,10 +227,11 @@ def test_merge_pr_has_non_breaking_change(
     assert len(created_comments) == 1
     assert (
         created_comments[0].body
-        == f"""**SQLMesh Bot Info**
-- PR Virtual Data Environment: hello_world_2
+        == f""":robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`
 <details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 {expected_prod_plan_summary}
 </details>
@@ -425,10 +426,11 @@ def test_merge_pr_has_non_breaking_change_diff_start(
     assert len(created_comments) == 1
     assert (
         created_comments[0].body
-        == f"""**SQLMesh Bot Info**
-- PR Virtual Data Environment: hello_world_2
+        == f""":robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`
 <details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 {expected_prod_plan}
 </details>
@@ -735,9 +737,9 @@ def test_merge_pr_has_no_changes(
     assert len(created_comments) == 1
     assert (
         created_comments[0].body
-        == f"""**SQLMesh Bot Info**
+        == f""":robot: **SQLMesh Bot Info** :robot:
 <details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 {expected_prod_plan_summary}
 </details>
@@ -921,8 +923,9 @@ def test_no_merge_since_no_deploy_signal(
     assert len(created_comments) == 1
     assert (
         created_comments[0].body
-        == """**SQLMesh Bot Info**
-- PR Virtual Data Environment: hello_world_2"""
+        == """:robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`"""
     )
 
     with open(github_output_file, "r", encoding="utf-8") as f:
@@ -1080,8 +1083,9 @@ def test_no_merge_since_no_deploy_signal_no_approvers_defined(
     assert len(created_comments) == 1
     assert (
         created_comments[0].body
-        == """**SQLMesh Bot Info**
-- PR Virtual Data Environment: hello_world_2"""
+        == """:robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`"""
     )
 
     with open(github_output_file, "r", encoding="utf-8") as f:
@@ -1257,10 +1261,13 @@ def test_deploy_comment_pre_categorized(
     assert len(created_comments) == 1
     assert (
         created_comments[0].body
-        == f"""**SQLMesh Bot Info**
-- PR Virtual Data Environment: hello_world_2
+        == f""":robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`
+- :arrow_forward: To apply this PR's plan to prod, comment:
+  - `/deploy`
 <details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 {expected_prod_plan}
 </details>
@@ -1613,10 +1620,11 @@ def test_overlapping_changes_models(
     assert len(created_comments) == 1
     assert (
         created_comments[0].body
-        == f"""**SQLMesh Bot Info**
-- PR Virtual Data Environment: hello_world_2
+        == f""":robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`
 <details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 {expected_prod_plan_summary}
 </details>
@@ -1791,10 +1799,11 @@ def test_pr_delete_model(
     assert len(created_comments) == 1
     assert (
         created_comments[0].body
-        == f"""**SQLMesh Bot Info**
-- PR Virtual Data Environment: hello_world_2
+        == f""":robot: **SQLMesh Bot Info** :robot:
+- :eyes: To **review** this PR's changes, use virtual data environment:
+  - `hello_world_2`
 <details>
-  <summary>Prod Plan Being Applied</summary>
+  <summary>:ship: Prod Plan Being Applied</summary>
 
 {expected_prod_plan_summary}
 </details>