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),
]
)
+
+
[email protected](
+ "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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
