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

remote: Collect ETag in response, optimistically send that back in future updates #1386

Closed
wants to merge 2 commits into from

Conversation

imjasonh
Copy link
Collaborator

@imjasonh imjasonh commented Jun 14, 2022

When pulling a manifest, if the response includes an ETag header, hold on to it, and send that back in an If-Match request header when putting that manifest back.

This should prevent some race conditions during read-modify-write cycles, if the registry respects the If-Match header, since the resource's etag will have changed when the other client updated it.

TODO: Add test.

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2022

Codecov Report

Merging #1386 (8eed56d) into main (12aeccc) will decrease coverage by 0.06%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #1386      +/-   ##
==========================================
- Coverage   74.19%   74.12%   -0.07%     
==========================================
  Files         113      113              
  Lines        8458     8474      +16     
==========================================
+ Hits         6275     6281       +6     
- Misses       1575     1584       +9     
- Partials      608      609       +1     
Impacted Files Coverage Δ
pkg/v1/mutate/image.go 67.37% <0.00%> (-1.86%) ⬇️
pkg/v1/partial/with.go 64.00% <ø> (ø)
pkg/v1/remote/write.go 64.51% <0.00%> (-0.32%) ⬇️
pkg/v1/remote/image.go 79.33% <50.00%> (-0.67%) ⬇️
pkg/v1/remote/descriptor.go 72.79% <69.23%> (+0.40%) ⬆️
pkg/v1/remote/index.go 71.27% <80.00%> (-0.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12aeccc...8eed56d. Read the comment docs.

@jonjohnsonjr
Copy link
Collaborator

Maybe it's time to have formal unwrapping?

I was thinking something more along the lines of #1387 for the partial stuff, but it might be better to have an actual unwrapping API (borrowed from https://go.dev/blog/go1.13-errors) so we can check "does this (or something it wraps) implement ETag?" instead of just returning an empty string.

@github-actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@oofnish
Copy link

oofnish commented Feb 7, 2023

Hello, I know this PR is currently closed, but I'm interested in picking up and running with the ETag support here. Is it just tests that are missing for this, or would the "Formal unwrapping" approach be a requirement for a MVP here?

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.

None yet

4 participants