-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Feat: artifact adds AI Model type #21691
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
Conversation
Signed-off-by: zhaoxinxin <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #21691 +/- ##
==========================================
+ Coverage 45.36% 46.42% +1.05%
==========================================
Files 244 253 +9
Lines 13333 14198 +865
Branches 2719 2924 +205
==========================================
+ Hits 6049 6591 +542
- Misses 6983 7257 +274
- Partials 301 350 +49
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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
...rc/app/base/project/repository/artifact/artifact-additions/artifact-additions.component.html
Outdated
Show resolved
Hide resolved
...rc/app/base/project/repository/artifact/artifact-additions/artifact-additions.component.html
Outdated
Show resolved
Hide resolved
.../src/app/base/project/repository/artifact/artifact-additions/artifact-additions.component.ts
Outdated
Show resolved
Hide resolved
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
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
Signed-off-by: zhaoxinxin <[email protected]>
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
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.
We should remove the top error message for models with no readme or license. Since this is too much errors everytime I open an artifact with no annotations. and according to spec annotations are optional. So we should not throw an error.
Just letting it here. We possibly can create another PR once this got merged 👍
Thanks
@bupd Good catch! I think we can raise another PR to handle this issue, avoid making this one overly large and difficult to review. |
Signed-off-by: zhaoxinxin <[email protected]>
Signed-off-by: zhaoxinxin <[email protected]>
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
...portal/src/app/base/project/repository/artifact/artifact-label/artifact-label.component.html
Outdated
Show resolved
Hide resolved
src/portal/src/app/base/project/repository/artifact/artifact-label/artifact-label.component.ts
Outdated
Show resolved
Hide resolved
...ortal/src/app/base/project/repository/artifact/artifact-additions/files/files.component.html
Outdated
Show resolved
Hide resolved
.../src/app/base/project/repository/artifact/artifact-additions/artifact-additions.component.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: zhaoxinxin <[email protected]>
@Liam-Zhao @chlins @OrlinVasilev @wy65701436 @renmaosheng I find it very sad that we ignore easily correctable rookie mistakes with excuse to fix it later another day, which we all know will never be fulfilled...
|
We've discussed this issue both online and offline and decided to address it in a separate pull request. The current PR has become quite large, has been open for a long time, and requires frequent synchronization with the latest changes. And it has already passed review. To keep things manageable, we'll tackle the minor UI issue in its own PR. Additionally, as we're approaching the FC stage, we can address minor issues between FC and the RC phases. Technically, the error handling tooltip is a shared component at the product level. If we want to customize the 404 error for fetching annotations, it would be best to discuss and review this separately to ensure alignment with our overall design principles. In my opinion, this is more of a UI-friendly enhancement and shouldn't block the PR from being merged. Would you mind creating an issue for it? We can then discuss whether we should do it and how to approach it. |
Thank you for contributing to Harbor!
Comprehensive Summary of your change
This pull request adds a UI display of artifact Model type, artifact-additions adds file and license display, and Model tag display
Issue being fixed
Fixed: #21229
Please indicate you've done the following: