-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(v2/dsl): support minio & s3 artifacts in v2 python component. Fixes #5838 #5909
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.
/lgtm
return _MINIO_LOCAL_MOUNT_PREFIX + self.uri[len('minio://'):] | ||
elif self.uri.startswith('s3://'): | ||
return _S3_LOCAL_MOUNT_PREFIX + self.uri[len('s3://'):] | ||
return None | ||
|
||
def _set_path(self, path): |
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.
nit: not related to your change, but I now feel like this method name is a bit confusing, we are setting self.uri but not path. Maybe we could rename this private method to _set_uri
while keep the path
property name unchanged. So it would be
@path.setter
def path(self, path):
self._set_uri(path)
You don't have to make the change though. This isn't related to your change. We may clean things up later.
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.
Agree with these points, I felt it was confusing too.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description of your changes:
Fixes #5838
This also fixes a bug @zijianjoy found in #5859 that minio artifacts are written to a wrong path, without the pipeline root as prefix.
Checklist: