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

feat: Public GC function of oci.Store #656

Merged
merged 25 commits into from
Jan 11, 2024
Merged

feat: Public GC function of oci.Store #656

merged 25 commits into from
Jan 11, 2024

Conversation

wangxiaoxuan273
Copy link
Contributor

@wangxiaoxuan273 wangxiaoxuan273 commented Dec 8, 2023 •
edited
Loading

Part of #472

@wangxiaoxuan273 wangxiaoxuan273 changed the title Public gc feat: Public GC function of oci.Store Dec 11, 2023
@wangxiaoxuan273 wangxiaoxuan273 marked this pull request as ready for review December 11, 2023 07:17
@codecov-commenter
Copy link

codecov-commenter commented Dec 11, 2023 •
edited by codecov bot
Loading

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (c34d275) 75.57% compared to head (20af59f) 75.71%.

Files Patch % Lines
content/oci/oci.go 77.61% 10 Missing and 5 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #656      +/-   ##
==========================================
+ Coverage   75.57%   75.71%   +0.13%     
==========================================
  Files          59       59              
  Lines        5692     5765      +73     
==========================================
+ Hits         4302     4365      +63     
- Misses       1023     1028       +5     
- Partials      367      372       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

qweeah
qweeah reviewed Dec 18, 2023
View reviewed changes
content/oci/oci.go Outdated Show resolved Hide resolved
@wangxiaoxuan273 wangxiaoxuan273 marked this pull request as draft December 18, 2023 06:21
@eiffel-fl eiffel-fl mentioned this pull request Dec 21, 2023
Merged
@wangxiaoxuan273 wangxiaoxuan273 marked this pull request as ready for review December 25, 2023 02:15
qweeah
qweeah reviewed Dec 25, 2023
View reviewed changes
internal/graph/memory.go Outdated Show resolved Hide resolved
Wwwsylvia
Wwwsylvia requested changes Dec 26, 2023
View reviewed changes
content/oci/oci.go Outdated Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
internal/graph/memory.go Outdated Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
shizhMSFT
shizhMSFT reviewed Dec 28, 2023
View reviewed changes
content/oci/oci.go Outdated Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
content/oci/oci.go Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
Wwwsylvia
Wwwsylvia reviewed Dec 28, 2023
View reviewed changes
internal/graph/memory.go Outdated Show resolved Hide resolved
Wwwsylvia
Wwwsylvia reviewed Dec 28, 2023
View reviewed changes
content/oci/oci.go Outdated Show resolved Hide resolved
@shizhMSFT shizhMSFT mentioned this pull request Dec 29, 2023
Open
@Wwwsylvia Wwwsylvia mentioned this pull request Dec 29, 2023
Merged
@eiffel-fl eiffel-fl mentioned this pull request Dec 29, 2023
Closed
eiffel-fl
eiffel-fl reviewed Dec 29, 2023
View reviewed changes
Copy link
Contributor

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

Hi!

This looks really cool!
I tested it and did not find any problem (I am still having blobs not deleted but I am thinking more and more this is because we are doing something wrong on our side).
I just have one comment regarding the TODO but nothing critical.

Best regards.

content/oci/oci.go Outdated Show resolved Hide resolved
eiffel-fl
eiffel-fl reviewed Jan 3, 2024
View reviewed changes
Copy link
Contributor

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

Hi!

I tested it and it works fine! I just found one small nit but unless I am missing something I think we are near the merge.

Best regards.

internal/graph/memory.go Outdated Show resolved Hide resolved
eiffel-fl
eiffel-fl approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

Hi!

I tested it and I like the new approach of using index.json!
I am still getting the same behavior but this is not related to this code (see #664).
I just found one small nit, otherwise, from my side, it is ready to go (note that I am not maintainer here, so just grey checkmark)!

Best regards.

content/oci/oci.go Outdated Show resolved Hide resolved
@wangxiaoxuan273
feat: dangling node detection for graph.Memory
85a91aa 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
resolved dangling tree issue
07e89df 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
removed storage changes
c921137 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
resolved some comments
5083aea 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
added test case for referrers
7b4c0b7 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
replaced walk with loop
a809ea2 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
used traverseIndex
4130046 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
change delete back
b2dd91c 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
added context check
a6adc33 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
revert change in memory
b85b5aa 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
fixed context issue
ca35c14 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
increase coverage
e18dd2f 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
increase coverage
b174f14 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
increase coverage
60624cb 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
increase coverage
bb899fa 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
remove extra set
f896e13 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
resolved comments
eb087c0 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
reload index
2b62edb 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
increase coverage
1e15e73 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
increase coverage
70afd40 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
increase coverage
fc450e1 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
increase coverage
a429bb4 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
minor fix
a6eadac 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
resolved comments
de79a2e 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
resolved comments
20af59f 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
Wwwsylvia
Wwwsylvia approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@Wwwsylvia Wwwsylvia left a comment

Choose a reason for hiding this comment

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

LGTM

@Wwwsylvia Wwwsylvia merged commit 3b1dd0e into oras-project:main Jan 11, 2024
9 checks passed
@wangxiaoxuan273 wangxiaoxuan273 deleted the public-gc branch January 11, 2024 06:07
@Wwwsylvia Wwwsylvia mentioned this pull request Jan 26, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qweeah qweeah qweeah left review comments

@Wwwsylvia Wwwsylvia Wwwsylvia approved these changes

@eiffel-fl eiffel-fl eiffel-fl approved these changes

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@sajayantony sajayantony Awaiting requested review from sajayantony sajayantony is a code owner

@SteveLasker SteveLasker Awaiting requested review from SteveLasker SteveLasker is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wangxiaoxuan273 @codecov-commenter @qweeah @Wwwsylvia @eiffel-fl @shizhMSFT