fix more regressions in package client, add test coverage #639
Conversation
The more structured error type is not used at the callsite, but causes a problem because the calling function can now return the non-nil empty error `error((*keppel.RegistryV2Error)(nil))`.
The API endpoint for monolithic blob uploads requires a slash at the end of the path. This trailing slash is required by the API specification, see `end-4b` in [1]. [1] https://github.com/opencontainers/distribution-spec/blob/v1.1.1/spec.md#endpoints
| Query: url.Values{"digest": []string{d.String()}}, | ||
| Headers: http.Header{ | ||
| "Content-Type": {"application/octet-stream"}, | ||
| "Content-Length": {strconv.FormatUint(uint64(len(contents)), 10)}, |
There was a problem hiding this comment.
This header must now be added manually because the testcase does not go through a real HTTP server (where it would normally be added automatically).
There was a problem hiding this comment.
That should probably be a comment
majewsky
force-pushed
the
repoclient-test-coverage
branch
from
0db6bf1 to
3a8427a
Compare
There was a problem hiding this comment.
Pull request overview
This pull request fixes regressions in the package client and adds comprehensive test coverage. The main issues addressed are: (1) an error handling bug where returning *keppel.RegistryV2Error could result in a non-nil empty error, and (2) ensuring the monolithic blob upload endpoint preserves the required trailing slash per the OCI distribution specification.
Changes:
- Fixed error handling in authentication to use
errorinstead of*keppel.RegistryV2Errorto prevent nil pointer issues - Added trailing slash preservation logic for API endpoints that require it
- Added Content-Length header to monolithic blob uploads for better HTTP compliance
- Added comprehensive test coverage for RepoClient upload and download operations
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/client/auth_challenge.go | Changed GetToken return type from *keppel.RegistryV2Error to error to fix nil error handling bug |
| internal/client/repo_client.go | Added trailing slash preservation and updated error handling to use error type consistently |
| internal/client/upload.go | Added Content-Length header to monolithic blob upload requests |
| internal/test/content.go | Added documentation for GenerateImageWithCustomConfig function |
| internal/client/repo_client_test.go | New comprehensive test file covering RepoClient upload and download operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
*keppel.RegistryV2ErrorfromAuthChallenge.GetToken()does not have any benefit, but causes aproblem because the calling function can now return the non-nil empty error
error((*keppel.RegistryV2Error)(nil)).end-4bin https://github.com/opencontainers/distribution-spec/blob/v1.1.1/spec.md#endpoints.