ambika-garg commented on code in PR #44696:
URL: https://github.com/apache/airflow/pull/44696#discussion_r1890555910


##########
providers/src/airflow/providers/microsoft/azure/operators/powerbi.py:
##########
@@ -126,7 +124,11 @@ def execute_complete(self, context: Context, event: 
dict[str, str]) -> Any:
         Relies on trigger to throw an exception, otherwise it assumes 
execution was successful.
         """
         if event:
-            if event["status"] == "error":
+            if (
+                event["status"] == "error"

Review Comment:
   I would go with Option 3, i.e divide the information into two fields: 
   `dataset_refresh_status`: Would contain the originial API status
   `status`: Reflects overall status (success/error)



##########
providers/src/airflow/providers/microsoft/azure/operators/powerbi.py:
##########
@@ -126,7 +124,11 @@ def execute_complete(self, context: Context, event: 
dict[str, str]) -> Any:
         Relies on trigger to throw an exception, otherwise it assumes 
execution was successful.
         """
         if event:
-            if event["status"] == "error":
+            if (
+                event["status"] == "error"

Review Comment:
   This approach would be more flexible and extensible.
   
   PowerBI Dataset refresh API returns 4 kinds of status:
   `Unknown` if the completion state is unknown or a refresh is in progress.
   `Completed` for a successfully completed refresh.
   `Failed` for an unsuccessful refresh (serviceExceptionJson will contain the 
error code).
   `Disabled` if the refresh is disabled by a selective refresh.
   
   So instead of applying `OR` on different dataset refresh statuses:
   I would make following changes:
   
   In `PowerBIDatasetRefreshStatus` class:
   ```python
   FAILURE_STATUSES = {FAILED, DISABLED}
   ```
   
   In `PowerBITrigger` class::
   ```python
   elif dataset_refresh_status in PowerBIDatasetRefreshStatus.FAILURE_STATUSES:
           yield TriggerEvent(
               {
                   "status": "error",
                   "dataset_refresh_status": dataset_refresh_status
                   "message": f"The dataset refresh {self.dataset_refresh_id} 
has {dataset_refresh_status}.",
                   "dataset_refresh_id": self.dataset_refresh_id,
               }
           )
           return
   ```



-- 
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]

Reply via email to