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: add Untag() to *oci.Store #647

Merged
merged 1 commit into from
Dec 21, 2023
Merged

feat: add Untag() to *oci.Store #647

merged 1 commit into from
Dec 21, 2023

Conversation

eiffel-fl
Copy link
Contributor

Hi!

oci.Store offers the possibility to tag images but not to untag them.
This is a problem when you want to remove images because you only want to untag a tag when several point to the same image (e.g. docker rmi and see inspektor-gadget/inspektor-gadget#2162 (review)).

I will add proper testing if we agree that this addition is useful to the package.

Best regards and thank you in advance.

@eiffel-fl eiffel-fl mentioned this pull request Nov 21, 2023
Merged
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (98758b7) 75.23% compared to head (73b2766) 75.32%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #647      +/-   ##
==========================================
+ Coverage   75.23%   75.32%   +0.08%     
==========================================
  Files          59       59              
  Lines        5480     5499      +19     
==========================================
+ Hits         4123     4142      +19     
  Misses       1001     1001              
  Partials      356      356

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

@wangxiaoxuan273
Copy link
Contributor

wangxiaoxuan273 commented Nov 23, 2023 •
edited
Loading

Hi @eiffel-fl, for a feature like this, we need more discussions and design. We encourage you to open an issue, so that everyone can discuss ideas and scenarios on the issue page.

Best regards.

@eiffel-fl eiffel-fl mentioned this pull request Nov 23, 2023
Closed
@eiffel-fl eiffel-fl force-pushed the francis-untag branch 4 times, most recently from 0643b35 to b31bb26 Compare December 4, 2023 13:55
@wangxiaoxuan273
Copy link
Contributor

Hi, I think your original implementation was good (before you change the input from reference to digest), how about changing it back and continue on that one?

@eiffel-fl eiffel-fl force-pushed the francis-untag branch 2 times, most recently from c6dcbf7 to 1213840 Compare December 13, 2023 13:44
@eiffel-fl
Copy link
Contributor Author

eiffel-fl commented Dec 13, 2023 •
edited
Loading

I polished the code and I added some tests, please let me know if further testing are needed (like a dedicated function).
I tried this newer version of the patch with Inspektor Gadget and I am still getting the behavior I aiming for!

wangxiaoxuan273
wangxiaoxuan273 reviewed Dec 14, 2023
View reviewed changes
content/oci/oci.go Outdated Show resolved Hide resolved
Wwwsylvia
Wwwsylvia reviewed Dec 14, 2023
View reviewed changes
content/resolver.go Outdated Show resolved Hide resolved
content/oci/oci.go Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
content/oci/oci.go Outdated Show resolved Hide resolved
@Wwwsylvia
Copy link
Member

Can we follow the Conventional Commits to have the PR title start with "feat:" ?

@wangxiaoxuan273
Copy link
Contributor

LGTM but I'm not a maintainer. For style consistency, can you change the PR title to feat: add Untag() to oci.Store?

Wwwsylvia
Wwwsylvia reviewed Dec 15, 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_test.go Outdated Show resolved Hide resolved
content/oci/oci_test.go Outdated Show resolved Hide resolved
content/oci/oci_test.go Outdated Show resolved Hide resolved
@eiffel-fl eiffel-fl changed the title Add Untag() to oci.Store. feat: Add Untag() to oci.Store. Dec 15, 2023
Wwwsylvia
Wwwsylvia reviewed Dec 19, 2023
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.

Sorry for the delayed review! Left a few suggestions regarding the unit tests. Let me know if anything is unclear.

content/oci/oci_test.go Outdated Show resolved Hide resolved
content/oci/oci_test.go Outdated Show resolved Hide resolved
content/oci/oci_test.go Outdated Show resolved Hide resolved
Wwwsylvia
Wwwsylvia reviewed Dec 20, 2023
View reviewed changes
content/oci/oci_test.go Outdated Show resolved Hide resolved
content/oci/oci_test.go Show resolved Hide resolved
content/oci/oci_test.go Outdated Show resolved Hide resolved
@eiffel-fl
feat: Add Untag() to oci.Store.
73b2766 
Signed-off-by: Francis Laniel <[email protected]>
Wwwsylvia
Wwwsylvia approved these changes Dec 21, 2023
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

@shizhMSFT shizhMSFT changed the title feat: Add Untag() to oci.Store. feat: add Untag() to *oci.Store Dec 21, 2023
@shizhMSFT shizhMSFT changed the title feat: add Untag() to *oci.Store feat: add Untag() to *oci.Store Dec 21, 2023
shizhMSFT
shizhMSFT approved these changes Dec 21, 2023
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT 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 5073458 into oras-project:main Dec 21, 2023
7 checks passed
@Wwwsylvia
Copy link
Member

@eiffel-fl Merged! Thank you for your contribution!

@eiffel-fl
Copy link
Contributor Author

Thank you for the reviews and the merge!

@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

@wangxiaoxuan273 wangxiaoxuan273 wangxiaoxuan273 left review comments

@qweeah qweeah qweeah left review comments

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@Wwwsylvia Wwwsylvia Wwwsylvia 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
@eiffel-fl @codecov-commenter @wangxiaoxuan273 @Wwwsylvia @qweeah @shizhMSFT