Skip to content

Add code to return errors from registries #3109

Merged
crosbymichael merged 2 commits intocontainerd:masterfrom
bainsy88:issue_3076
Apr 4, 2019
Merged

Add code to return errors from registries #3109
crosbymichael merged 2 commits intocontainerd:masterfrom
bainsy88:issue_3076

Conversation

@bainsy88
Copy link
Copy Markdown
Contributor

If there are no errors in the correct format expected from docker Registry it then it falls back to the old behaviour
#3076

Signed-off-by: Jack Baines jack.baines@uk.ibm.com

@estesp
Copy link
Copy Markdown
Member

estesp commented Mar 19, 2019

CI commit check failed due to "subject > 90 chars"; commit messages are expected in the form:

short title max 90 chars

longer description

Signed-off-by: First Last <email@address.com>

@bainsy88 bainsy88 force-pushed the issue_3076 branch 4 times, most recently from 0b64d2c to c9aa764 Compare March 19, 2019 21:05
Docker registries return errors in a know format so this change now checks for these
errors and returns the message field. If the error is not in the expected format fall
back to the original behaviour.

containerd#3076

Signed-off-by: Jack Baines <jack.baines@uk.ibm.com>
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #3109 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3109     +/-   ##
=========================================
+ Coverage   43.59%   43.69%   +0.1%     
=========================================
  Files         104      104             
  Lines       11135    11142      +7     
=========================================
+ Hits         4854     4869     +15     
+ Misses       5545     5537      -8     
  Partials      736      736
Flag Coverage Δ
#linux 47.65% <100%> (+0.07%) ⬆️
#windows 40.59% <100%> (+0.12%) ⬆️
Impacted Files Coverage Δ
remotes/docker/fetcher.go 65.65% <100%> (+11.3%) ⬆️

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 9ab4c8c...908b771. Read the comment docs.

1 similar comment
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #3109 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3109     +/-   ##
=========================================
+ Coverage   43.59%   43.69%   +0.1%     
=========================================
  Files         104      104             
  Lines       11135    11142      +7     
=========================================
+ Hits         4854     4869     +15     
+ Misses       5545     5537      -8     
  Partials      736      736
Flag Coverage Δ
#linux 47.65% <100%> (+0.07%) ⬆️
#windows 40.59% <100%> (+0.12%) ⬆️
Impacted Files Coverage Δ
remotes/docker/fetcher.go 65.65% <100%> (+11.3%) ⬆️

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 9ab4c8c...908b771. Read the comment docs.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 19, 2019

Codecov Report

Merging #3109 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3109     +/-   ##
=========================================
+ Coverage   43.59%   43.69%   +0.1%     
=========================================
  Files         104      104             
  Lines       11135    11142      +7     
=========================================
+ Hits         4854     4869     +15     
+ Misses       5545     5537      -8     
  Partials      736      736
Flag Coverage Δ
#linux 47.65% <100%> (+0.07%) ⬆️
#windows 40.59% <100%> (+0.12%) ⬆️
Impacted Files Coverage Δ
remotes/docker/fetcher.go 65.65% <100%> (+11.3%) ⬆️

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 9ab4c8c...d15832a. Read the comment docs.

"strings"

"github.com/docker/distribution/registry/api/errcode"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove this empty line ?

Copy link
Copy Markdown
Contributor Author

@bainsy88 bainsy88 Mar 21, 2019

Choose a reason for hiding this comment

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

That new line is added by goimport which is run as part of the CI tests, so I believe it would cause the travis build to fail if I remove it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if you add it after containerd/containerd/log you should be able to verify with gofmt that that would allow all external imports to be in the same section (which is pretty normal) rather than having it stand by itself. goimport isn't always as smart about combining these sections.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry I was looking at the _test file when I first read this. There is an extra space as you say. Will fix

}

// New set of test to test new error cases
func Test_dockerFetcher_open(t *testing.T) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks a bit python-esque? :) Maybe TestDockerFetcherOpen?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure can change

-Fix whitespace on imports
-Fix test case naming

Signed-off-by: Jack Baines <jack.baines@uk.ibm.com>
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@Ace-Tang
Copy link
Copy Markdown
Contributor

LGTM

@crosbymichael
Copy link
Copy Markdown
Member

LGTM

@crosbymichael crosbymichael merged commit 9bc2315 into containerd:master Apr 4, 2019
crosbymichael added a commit to crosbymichael/containerd that referenced this pull request Apr 4, 2019
Update to containerd#3109

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
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.

6 participants