Skip to content

Comments

fix: added warning for dangling referrers index deletion#619

Merged
shizhMSFT merged 4 commits intonotaryproject:mainfrom
patrickzheng200:fix
Apr 13, 2023
Merged

fix: added warning for dangling referrers index deletion#619
shizhMSFT merged 4 commits intonotaryproject:mainfrom
patrickzheng200:fix

Conversation

@patrickzheng200
Copy link
Contributor

@patrickzheng200 patrickzheng200 commented Apr 6, 2023

This PR tries to resolve #615.

added warning for dangling referrers index deletion
1e7380a 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
update
4ae0f85 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2023 •
edited
Loading

Codecov Report

Merging #619 (ee4cb98) into main (0ec3b3d) will decrease coverage by 0.15%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #619      +/-   ##
==========================================
- Coverage   34.43%   34.28%   -0.15%     
==========================================
  Files          32       32              
  Lines        1844     1852       +8     
==========================================
  Hits          635      635              
- Misses       1188     1196       +8     
  Partials       21       21
Impacted Files Coverage Δ
cmd/notation/sign.go 37.39% <0.00%> (-2.80%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

priteshbandi
priteshbandi reviewed Apr 6, 2023
View reviewed changes
@sajayantony
Copy link
Contributor

sajayantony commented Apr 6, 2023

Should the exit code be zero? /cc @toddysm

@toddysm
Copy link
Contributor

toddysm commented Apr 6, 2023

@sajayantony I believe so and avoid braking any scripts.

Also, is this the only message we will show:

Warning: failed to delete dangling referrers index sha256:xxx for referrers tag sha256-1ba1561: DELETE "https://ghcr.io/v2/demo-user/python/manifests/sha256:e1b7d6": response status code 405: unsupported: The operation is unsupported.

I believe there should still be a confirmation that the operation succeeded and the signature got attached to the artifact. We should show the standard message we show when signature is attached + the warning.

qweeah
qweeah reviewed Apr 7, 2023
View reviewed changes
cmd/notation/sign.go
signatureManifestImage = "image"
)

const referrersTagSchemaDeleteError = "failed to delete dangling referrers index"
Copy link
Contributor

@qweeah qweeah Apr 7, 2023

Choose a reason for hiding this comment

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

@shizhMSFT @Wwwsylvia Since this error is generated based on oras-go-specific procedure, should oras-go export this error so caller can implement upon?

Copy link
Contributor

@qweeah qweeah Apr 7, 2023

Choose a reason for hiding this comment

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

It's beyond this PR's scope, I created an issue for tracking, let's move discussion there.

Copy link
Contributor

@Wwwsylvia Wwwsylvia Apr 7, 2023

Choose a reason for hiding this comment

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

Yeah we need to think about how to improve oras-go for such use case.

Copy link
Contributor

@shizhMSFT shizhMSFT Apr 11, 2023

Choose a reason for hiding this comment

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

Once oras-go is updated and have a new release, we can update here. But for now, let's do a string match.

@patrickzheng200
Copy link
Contributor Author

patrickzheng200 commented Apr 7, 2023

Should the exit code be zero? /cc @toddysm

Yes, the exit code should be 0. Once the warning is printed out, Successfully signed... is printed out and finally return nil.

@patrickzheng200
Copy link
Contributor Author

patrickzheng200 commented Apr 7, 2023 •
edited
Loading

@sajayantony I believe so and avoid braking any scripts.

Also, is this the only message we will show:

Warning: failed to delete dangling referrers index sha256:xxx for referrers tag sha256-1ba1561: DELETE "https://ghcr.io/v2/demo-user/python/manifests/sha256:e1b7d6": response status code 405: unsupported: The operation is unsupported.

I believe there should still be a confirmation that the operation succeeded and the signature got attached to the artifact. We should show the standard message we show when signature is attached + the warning.

This warning message is an addition to the standard result. Can refer this in the code.

updated per code review
ee4cb98 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
qweeah
qweeah reviewed Apr 7, 2023
View reviewed changes
@patrickzheng200 patrickzheng200 requested a review from qweeah April 7, 2023 07:28
@patrickzheng200
Copy link
Contributor Author

patrickzheng200 commented Apr 7, 2023

Closing this PR due to suggestion here: #615 (comment).
IMO, we should think through this process again before implementation.

@patrickzheng200
Copy link
Contributor Author

patrickzheng200 commented Apr 10, 2023

Reopened based on discussion.

@patrickzheng200 patrickzheng200 mentioned this pull request Apr 10, 2023
Closed
update
4f7a677 
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
priteshbandi
priteshbandi approved these changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@priteshbandi priteshbandi 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 approved these changes Apr 11, 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

@shizhMSFT shizhMSFT merged commit 2841abe into notaryproject:main Apr 13, 2023
@patrickzheng200 patrickzheng200 deleted the fix branch April 13, 2023 04:18
7h3-3mp7y-m4n pushed a commit to 7h3-3mp7y-m4n/notation that referenced this pull request Mar 29, 2025
@7h3-3mp7y-m4n
fix: added warning for dangling referrers index deletion (notaryproje…
d1b5264 
…ct#619)

This PR tries to resolve notaryproject#615.

---------

Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
FeynmanZhou pushed a commit to FeynmanZhou/notation that referenced this pull request May 15, 2025
@FeynmanZhou
fix: added warning for dangling referrers index deletion (notaryproje…
ba239f4 
…ct#619)

This PR tries to resolve notaryproject#615.

---------

Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@priteshbandi priteshbandi priteshbandi approved these changes

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

@JeyJeyGao JeyJeyGao Awaiting requested review from JeyJeyGao

@justincormack justincormack Awaiting requested review from justincormack

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

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

@SteveLasker SteveLasker Awaiting requested review from SteveLasker

@qweeah qweeah Awaiting requested review from qweeah

+2 more reviewers

@Wwwsylvia Wwwsylvia Wwwsylvia left review comments

@byronchien byronchien byronchien approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve the error msg for OCI v1.0 registries or registries that don't support deletion API

9 participants

@patrickzheng200 @codecov-commenter @sajayantony @toddysm @qweeah @priteshbandi @Wwwsylvia @byronchien @shizhMSFT