[GitHub] [dolphinscheduler] zhongjiajie commented on a diff in pull request #10551: [Feature-10486][Python] Python-gate support the use of full-name definitions for resources.

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

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


##########
dolphinscheduler-python/pydolphinscheduler/tests/core/test_task.py:
##########
@@ -47,14 +47,14 @@
         (
             {
                 "local_params": ["foo", "bar"],
-                "resource_list": ["foo", "bar"],
+                "resource_list": [{"id": 1}, {"id": 2}],
                 "dependence": {"foo", "bar"},
                 "wait_start_timeout": {"foo", "bar"},
                 "condition_result": {"foo": ["bar"]},
             },
             {
                 "localParams": ["foo", "bar"],
-                "resourceList": ["foo", "bar"],
+                "resourceList": [{"id": 1}, {"id": 2}],

Review Comment:
   I find out we just add style `[{"key": val}, {"key": val}]` for test, can we also add `["foo", "bar"]` type resource for test and mock the function `query_resource` return values? I am not sure whether we can add this case to this function or not, but if we can not, please add new test function to cover it



##########
dolphinscheduler-python/pydolphinscheduler/tests/core/test_task.py:
##########
@@ -241,3 +241,34 @@ def test_add_duplicate(caplog):
                 re.findall("already in process definition", caplog.text),
             ]
         )
+
+
+@pytest.mark.parametrize(
+    "resources, expect",
+    [
+        (
+            ["/dev/test.py"],
+            [{"id": 1}],
+        ),
+        (
+            ["/dev/test.py", {"id": 2}],
+            [{"id": 1}, {"id": 2}],
+        ),
+    ],
+)
+@patch(
+    "pydolphinscheduler.core.task.Task.gen_code_and_version",
+    return_value=(123, 1),
+)
+@patch(
+    "pydolphinscheduler.core.task.Task.query_resource",
+    return_value=({"id": 1, "name": "/dev/test.py"}),
+)
+def test_python_resource_list(mock_code_version, mock_resource, resources, expect):

Review Comment:
   Thanks for adding this tests



##########
dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/python/PythonGatewayTest.java:
##########
@@ -83,6 +92,37 @@ public void testGetDependentInfo() {
         Assert.assertEquals((long) result.get("taskDefinitionCode"), taskDefinition.getCode());
     }
 
+
+    @Test
+    public void testQueryResourcesFileInfo() {

Review Comment:
   Great, thanks for adding tests for it



##########
dolphinscheduler-python/pydolphinscheduler/src/pydolphinscheduler/core/task.py:
##########
@@ -170,6 +170,17 @@ def process_definition(self, process_definition: Optional[ProcessDefinition]):
         """Set attribute process_definition."""
         self._process_definition = process_definition
 
+    @property
+    def resource_list(self) -> List:
+        """Get task define attribute `resource_list`."""
+        resources = list()
+        for resource in self._resource_list:
+            if type(resource) == str:
+                resources.append(self.query_resource(resource))
+            elif type(resource) == dict and resource.get("id") is not None:
+                resources.append(resource)

Review Comment:
   can we use `warnings.warn` to alert users this feat will is deprecation, should be use `List[str]` instead, and will be remove in version `3.2.0`?



##########
dolphinscheduler-python/pydolphinscheduler/src/pydolphinscheduler/core/task.py:
##########
@@ -170,6 +170,17 @@ def process_definition(self, process_definition: Optional[ProcessDefinition]):
         """Set attribute process_definition."""
         self._process_definition = process_definition
 
+    @property
+    def resource_list(self) -> List:
+        """Get task define attribute `resource_list`."""
+        resources = list()
+        for resource in self._resource_list:
+            if type(resource) == str:
+                resources.append(self.query_resource(resource))
+            elif type(resource) == dict and resource.get("id") is not None:
+                resources.append(resource)
+        return [{"id": r.get("id")} for r in resources]

Review Comment:
   could you add `"id"` to files constants? https://github.com/apache/dolphinscheduler/blob/80ebe4a33431c88c7514b3ad0d9ffdf645e1bba2/dolphinscheduler-python/pydolphinscheduler/src/pydolphinscheduler/constants.py



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