[GitHub] [dolphinscheduler] zhongjiajie commented on a diff in pull request #9773: [Improvement-9759][Python] Specify warning group when deploy process-definition via python-gate

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

zhongjiajie commented on code in PR #9773:
URL: https://github.com/apache/dolphinscheduler/pull/9773#discussion_r859328224


##########
dolphinscheduler-python/pydolphinscheduler/src/pydolphinscheduler/core/process_definition.py:
##########
@@ -100,6 +111,8 @@ def __init__(
         project: Optional[str] = configuration.WORKFLOW_PROJECT,
         tenant: Optional[str] = configuration.WORKFLOW_TENANT,
         worker_group: Optional[str] = configuration.WORKFLOW_WORKER_GROUP,
+        warning_type: Optional[str] = WarningType.NONE,
+        warning_group_id: Optional[int] = 0,

Review Comment:
   could you add `warning type` to our `default_config.yaml` and then use the configuration as default value just like `configuration.WORKFLOW_WORKER_GROUP` and `configuration.WORKFLOW_TENANT` https://github.com/apache/dolphinscheduler/blob/59a026d897d5fa3599dd58e932d164370a63b5f4/dolphinscheduler-python/pydolphinscheduler/src/pydolphinscheduler/core/configuration.py#L183-L185



##########
dolphinscheduler-python/pydolphinscheduler/src/pydolphinscheduler/core/process_definition.py:
##########
@@ -113,6 +126,8 @@ def __init__(
         self._project = project
         self._tenant = tenant
         self.worker_group = worker_group
+        self.warning_type = warning_type

Review Comment:
   If we read the value from `configration` file, we better check the input value before we assigned to `self.warning_type`. BTW, better and tests for this
   ```suggestion
           if warning_type.strip().upper() not in ("FAILURE", "SUCCESS", "ALL", "NONE"):
               raise PyDSParamException("Parameter `warning_type` with unexpect value `%s`", warning_type)
           else:
               self.warning_type = warning_type.strip().upper()
   ```



##########
dolphinscheduler-python/pydolphinscheduler/src/pydolphinscheduler/core/process_definition.py:
##########
@@ -113,6 +126,8 @@ def __init__(
         self._project = project
         self._tenant = tenant
         self.worker_group = worker_group
+        self.warning_type = warning_type

Review Comment:
   And then we could remove class `WarningType` in this file



-- 
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@dolphinscheduler.apache.org

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