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.

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

support nydus as a accessory #18953

Merged
merged 4 commits into from
Jul 20, 2023
Merged

support nydus as a accessory #18953

merged 4 commits into from
Jul 20, 2023

Conversation

wy65701436
Copy link
Contributor

@wy65701436 wy65701436 commented Jul 19, 2023

Recognize nydus image(with subject) as a kind of accessory and built the releationship with subject manifest

Thank you for contributing to Harbor!

Comprehensive Summary of your change

Issue being fixed

Fixes #(issue)

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

@wy65701436 wy65701436 requested a review from a team as a code owner July 19, 2023 08:50
Recognize nydus image(with subject) as a kind of accessory and built the releationship with subject manifest

Signed-off-by: wang yan <[email protected]>
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #18953 (be61ae5) into main (5e4163b) will decrease coverage by 0.02%.
The diff coverage is 46.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #18953      +/-   ##
==========================================
- Coverage   67.42%   67.40%   -0.02%     
==========================================
  Files         994      993       -1     
  Lines      108825   108768      -57     
  Branches     2747     2747              
==========================================
- Hits        73376    73316      -60     
- Misses      31511    31520       +9     
+ Partials     3938     3932       -6     
Flag Coverage Δ
unittests 67.40% <46.66%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/server/middleware/subject/subject.go 49.31% <46.66%> (-2.36%) ⬇️

... and 7 files with indirect coverage changes

Copy link
Member

@chlins chlins left a comment

Choose a reason for hiding this comment

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

lgtm

layers := manifest.Layers
if len(layers) != 0 {
desc := layers[len(layers)-1]
if desc.Annotations == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need tests cover this if branch

_, hasAnno := desc.Annotations[layerAnnotationNydusBootstrap]
return hasAnno
}
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

and also test case to cover this return false statement here.

Copy link
Contributor

@zyyw zyyw left a comment

Choose a reason for hiding this comment

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

Except the test to cover more logic in isNydusImage method, it looks good to me.

@wy65701436 wy65701436 merged commit ce89363 into goharbor:main Jul 20, 2023
MinerYang pushed a commit that referenced this pull request Jul 24, 2023
Recognize nydus image(with subject) as a kind of accessory and built the releationship with subject manifest

Signed-off-by: wang yan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants