Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Proposal for enhanced default processor to handle user-defined artifact like Machine Learning artifacts #143

Merged
merged 16 commits into from
Aug 27, 2020
Merged

Proposal for enhanced default processor to handle user-defined artifact like Machine Learning artifacts #143

merged 16 commits into from
Aug 27, 2020

Conversation

hyy0322
Copy link
Contributor

@hyy0322 hyy0322 commented May 31, 2020 •
edited
Loading

Signed-off-by: Yiyang Huang huangyiyang.huangyy@bytedance.com

@hyy0322 hyy0322 force-pushed the artifact-processor-extender branch 3 times, most recently from 33e62b1 to 258bda8 Compare May 31, 2020 08:56
@gaocegege gaocegege mentioned this pull request May 31, 2020
Closed
@gaocegege
Copy link
Contributor

Proposal Preview: proposals/artifact-processor-extender.md

/cc @zhujian7 @gaocegege @hainingzhang

/assign @xaleeks @steven-zou

@gaocegege
Copy link
Contributor

/cc @ddysher @codeflitting

@hyy0322 hyy0322 force-pushed the artifact-processor-extender branch from 258bda8 to ace1fda Compare May 31, 2020 09:12
@reasonerjt
Copy link
Contributor

reasonerjt commented Jun 3, 2020 •
edited
Loading

Hi,

Thanks for the proposal.

I find the framework proposed here may be a bit over-engineering at least for current requirements. It also introduces new attack-surface, for example the robot accounts in the external processors may be leaked and a malicious user can use it to pull artifacts from Harbor.

If we define a schema in the config blob of an artifact, it seems the processor extender is not very necessary?

i.e. if a in-tree processor and extract data as a pre-defined contract the artifact developer will only have to write the config JSON object in a certain format, then he will not need to implement his processor and we don't need to introduce yet another framework to manage external service registration in Harbor?

@gaocegege
Copy link
Contributor

Currently, it may work. In our use case, we now just have one possible addition: README.

In the future, we will support a new type ML Dataset. And we will keep a tiny part of the dataset in one blob layer as one addition. It will be used to show the directory struture of the dataset. These kinds of artifacts require higher extensibility, I think.

We can have the detailed dicusstions in the community meeting today 🍻 We are open to the decision. If the community members think it is too far, we can keep a in-tree common processor instead.

@gaocegege
Copy link
Contributor

@reasonerjt After a detailed review, I think in-tree processor is also one possible approach. We will refine the proposal soon.

@hyy0322 hyy0322 force-pushed the artifact-processor-extender branch 3 times, most recently from d02cf2d to af8eafb Compare June 7, 2020 15:50
@hyy0322 hyy0322 force-pushed the artifact-processor-extender branch 4 times, most recently from f5517b2 to e17617f Compare June 11, 2020 15:28
@hyy0322 hyy0322 force-pushed the artifact-processor-extender branch from e17617f to f3f79c9 Compare June 15, 2020 15:09
@hyy0322 hyy0322 force-pushed the artifact-processor-extender branch from f3f79c9 to 7349e0a Compare June 17, 2020 13:48
@xaleeks xaleeks changed the title Proposal for artifact processor extender Proposal for artifact processor extender to handle Machine Learning artifacts Jun 18, 2020
@hyy0322 hyy0322 force-pushed the artifact-processor-extender branch 3 times, most recently from c28fb4b to ffa1ed1 Compare June 27, 2020 10:12
steven-zou
steven-zou reviewed Jul 30, 2020
View reviewed changes
Copy link
Contributor

@steven-zou steven-zou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments

proposals/enhanced-default-processor.md Outdated Show resolved Hide resolved
proposals/enhanced-default-processor.md Outdated Show resolved Hide resolved
@steven-zou
Copy link
Contributor 

io.goharbor.artifact.v1alpha1.icon
io.goharbor.artifact.v1alpha1.skip-list

@reasonerjt reasonerjt mentioned this pull request Aug 4, 2020
Closed
@hyy0322 hyy0322 force-pushed the artifact-processor-extender branch from 2e5ebbf to ad8945f Compare August 5, 2020 07:06
gaocegege
gaocegege reviewed Aug 5, 2020
View reviewed changes
Copy link
Contributor

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@gaocegege
Copy link
Contributor

The PR is merged, please update the proposal according to our implementation @hyy0322

👍

hyy0322 and others added 15 commits August 17, 2020 17:48
@hyy0322
feat: artifact processor extender proposal
fe072d4 
Signed-off-by: Yiyang Huang <huangyiyang@caicloud.io>
@gaocegege @hyy0322
feat(proposals): Refine
ec2c89f 
Signed-off-by: Ce Gao <ce.gao@outlook.com>
@gaocegege @hyy0322
fix: Fix comment
ad06df8 
Signed-off-by: Ce Gao <ce.gao@outlook.com>
@hyy0322
feat(proposals): Add in-tree design
d9d51bb 
Signed-off-by: Yiyang Huang <huangyiyang@caicloud.io>
@gaocegege @hyy0322
feat: Remove out-of-tree implementation
22a4135 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege @hyy0322
feat: Update
724a52c 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege @hyy0322
feat: Update TOC
ad4587a 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege @hyy0322
Update
ddb26c9 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege @hyy0322
feat: Update
65d1785 
Signed-off-by: Ce Gao <ce.gao@outlook.com>
@gaocegege @hyy0322
feat: Add JSON schema
3af6644 
Signed-off-by: Ce Gao <ce.gao@outlook.com>
@hyy0322
feat: update proposal
8f21721 
Signed-off-by: Yiyang Huang <huangyiyang@caicloud.io>
@gaocegege @hyy0322
feat: Use annotations
451623e 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege @hyy0322
fix: Address comments
ec25183 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@hyy0322
fix: Address comments
06a0bfe 
Signed-off-by: Yiyang Huang <huangyiyang@caicloud.io>
@hyy0322
feat: add annotation processor
a07d255 
Signed-off-by: Yiyang Huang <huangyiyang.huangyy@bytedance.com>
@hyy0322 hyy0322 force-pushed the artifact-processor-extender branch from ad8945f to a07d255 Compare August 17, 2020 10:27
@hyy0322
Copy link
Contributor Author

hyy0322 commented Aug 17, 2020

Proposal updated.

steven-zou
steven-zou approved these changes Aug 27, 2020
View reviewed changes
Copy link
Contributor

@steven-zou steven-zou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved

@steven-zou
Copy link
Contributor

According to the lazy consensus principle, merge this proposal PR.

The feature will be delivered in V2.1.

@steven-zou steven-zou merged commit 570e42e into goharbor:master Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reasonerjt reasonerjt reasonerjt left review comments

@gaocegege gaocegege gaocegege left review comments

@ywk253100 ywk253100 ywk253100 left review comments

@zhujian7 zhujian7 zhujian7 left review comments

@xaleeks xaleeks xaleeks left review comments

@steven-zou steven-zou steven-zou approved these changes

Assignees
No one assigned
Labels
area/OCI-related enhancement New feature or request kind/proposal release/2.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hyy0322 @gaocegege @reasonerjt @renmaosheng @zhujian7 @ywk253100 @steven-zou @xaleeks