-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature-16396][storage-plugin] Tencent Cloud COS Storage Plugin #16565
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we don't have any tencent cos support for integration testing. I will -1 on this until we can find a way to test against this in out UT, IT and E2E.
dolphinscheduler-storage-plugin/dolphinscheduler-storage-cos/src/test/resources/logback.xml
Show resolved
Hide resolved
public static final String TENCENT_CLOUD_ACCESS_KEY_ID = "resource.tencent.cloud.access.key.id"; | ||
public static final String TENCENT_CLOUD_ACCESS_KEY_SECRET = "resource.tencent.cloud.access.key.secret"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to storage plugin module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your valuable opinions.
But I don't think placing these Tencent Credential Constants in the storage plugin is a good idea.
Cloud providers (Azure, Alibaba, Huawei, and Tencent) offer not only storage services but also other types of services.
Therefore, I believe Tencent Credential Constants should remain in task-plugin/task-api/org.apache.dolphinscheduler.plugin.task.api.TaskConstants
, alongside the credential constants for other cloud providers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to storage plugin module
agree with @ruanwenjun ,Although the keys and secrets of various storage types are currently placed in the task-api module, I think this is unreasonable and should be placed in their own storage modules or in the storage-api module WDYT @Mighten cc @SbloodyS @rickchengx
String cosKey = transformAbsolutePathToCOSKey(srcFilePath); | ||
|
||
File dstFile = new File(dstFilePath); | ||
if (dstFile.isDirectory()) { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
|
||
File dstFile = new File(dstFilePath); | ||
if (dstFile.isDirectory()) { | ||
Files.delete(dstFile.toPath()); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
f444eb8
to
ea644bc
Compare
* Fixed SonarCloud Quality Readability B (required >= A) * Added missing license files * YAML-ized Tencent COS configuration from common.properties * Added remote logger support with Tencent COS
ea644bc
to
40e5ff2
Compare
- Removed redundant dependency: `bcprov-jdk15on-1.69.jar`
Quality Gate failedFailed conditions |
Purpose of the pull request
This pull request adds a Storage Plugin to the DS Resource Center to support Tencent Cloud COS and aims to close #16396 .
Brief change log
dolphinscheduler-storage-cos
indolphinscheduler-storage-plugin
Verify this pull request
Pull Request Notice
Pull Request Notice
If your pull request contain incompatible change, you should also add it to
docs/docs/en/guide/upgrede/incompatible.md