Skip to content

Conversation

dmathieu
Copy link
Contributor

No description provided.

Copy link
Contributor

@caervs caervs left a comment

Choose a reason for hiding this comment

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

LGTM with one nit. Thanks!

w.Header().Set("Docker-Content-Digest", dgst.String())
w.Header().Set("Etag", dgst.String())

if r.Method == "HEAD" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would use http.MethodHead so we have some validation of the value

return err
}

w.Header().Set("Content-Length", strconv.FormatInt(desc.Size, 10))
Copy link
Contributor

Choose a reason for hiding this comment

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

These headers re-appear quite a bit

Makes mental note that we should have a utility function for setting them

@caervs caervs requested a review from manishtomar May 16, 2019 17:08
Damien Mathieu added 2 commits June 25, 2019 09:30
Signed-off-by: Damien Mathieu <dmathieu@salesforce.com>
A registry pointing to ECR is having issues if we try loading the blob

Signed-off-by: Damien Mathieu <dmathieu@salesforce.com>
@codecov
Copy link

codecov bot commented Jun 25, 2019

Codecov Report

Merging #2921 into master will increase coverage by 0.04%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2921      +/-   ##
=========================================
+ Coverage   60.45%   60.5%   +0.04%     
=========================================
  Files         102     102              
  Lines        8002    8016      +14     
=========================================
+ Hits         4838    4850      +12     
  Misses       2515    2515              
- Partials      649     651       +2
Flag Coverage Δ
#linux 60.5% <73.33%> (+0.04%) ⬆️
Impacted Files Coverage Δ
registry/client/repository.go 58.62% <73.33%> (+0.79%) ⬆️

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 79f6bcb...c5d5f93. Read the comment docs.

@caervs caervs merged commit 90dfea7 into distribution:master Jun 26, 2019
@dmathieu dmathieu deleted the repository-serve-blob branch June 26, 2019 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants