Conversation
There was a problem hiding this comment.
Pull request overview
This PR performs final cleanup and refactoring following previously merged changes. The primary focus is reorganizing package structure, implementing progress callback throttling, and removing obsolete code.
Changes:
- Refactored import paths: moved
drsandhashpackages fromindexd/drsandindexd/hashto top-level packages - Implemented progress callback throttling to reduce callback frequency using
MinChunkSizethreshold in both upload and download operations - Reorganized code: moved
S3Metatype tos3utilspackage, movedUpsertIndexdRecordto dedicated file, and fixed package declaration bug ins3utils/s3_utils.go
Reviewed changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| upload/upload.go | Updated import path for drs package |
| upload/progress_reader.go | Added throttling mechanism for progress callbacks using bytesSinceReport field |
| upload/multipart_test.go | Added Requestor() method to mock interface |
| s3utils/s3_utils.go | Fixed package declaration from "indexd" to "s3utils" and added S3Meta type |
| mocks/mock_indexd.go | Updated drs package import path |
| mocks/mock_gen3interface.go | Added Requestor() and Sower() mock methods |
| mocks/mock_fence.go | Added RefreshToken() and UserPing() mock methods |
| indexd/upsert.go | New file containing UpsertIndexdRecord method extracted from add_url.go |
| indexd/types_test.go | Updated import paths for drs and hash packages |
| indexd/types.go | Updated import paths and removed S3Meta type |
| indexd/tests/mock_servers_test.go | Deleted obsolete test file |
| indexd/tests/client_write_test.go.todo | Deleted todo test file |
| indexd/tests/client_read_test.go.todo | Deleted todo test file |
| indexd/tests/add-url-integration_test.go | Deleted obsolete integration test |
| indexd/records.go | Updated hash package import path |
| indexd/convert.go | Updated drs package import path |
| indexd/client_test.go | Updated import paths for drs and hash packages |
| indexd/client.go | Removed unused imports and AddURL interface method |
| indexd/add_url.go | Deleted file (functionality moved to upsert.go) |
| hash/hash_test.go | New test file for hash package |
| hash/hash.go | New file containing hash types and conversion functions |
| drs/types.go | Updated import path for hash package |
| drs/object_builder_test.go | New test file for ObjectBuilder |
| drs/object_builder.go | New file containing ObjectBuilder type |
| drs/drs.go | Updated hash package import path |
| download/transfer_test.go | Updated imports and added Requestor() method (with duplicate definition issue) |
| download/progress_writer.go | Added throttling mechanism for progress callbacks using bytesSinceReport field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (f *fakeGen3Download) Fence() fence.FenceInterface { return &fakeFence{doFunc: f.doFunc} } | ||
| func (f *fakeGen3Download) Indexd() indexd.IndexdInterface { return &fakeIndexd{doFunc: f.doFunc} } | ||
| func (f *fakeGen3Download) Sower() sower.SowerInterface { return nil } | ||
| func (f *fakeGen3Download) Requestor() requestor.RequestorInterface { return nil } |
There was a problem hiding this comment.
This method is defined twice in this file: once on line 41 returning nil, and again on lines 221-223 returning a fakeRequestor implementation. The second definition on lines 221-223 will override the first one, making the definition on line 41 redundant and potentially confusing. Remove the definition on line 41 to avoid confusion.
| func (f *fakeGen3Download) Fence() fence.FenceInterface { return &fakeFence{doFunc: f.doFunc} } | |
| func (f *fakeGen3Download) Indexd() indexd.IndexdInterface { return &fakeIndexd{doFunc: f.doFunc} } | |
| func (f *fakeGen3Download) Sower() sower.SowerInterface { return nil } | |
| func (f *fakeGen3Download) Requestor() requestor.RequestorInterface { return nil } | |
| func (f *fakeGen3Download) Fence() fence.FenceInterface { return &fakeFence{doFunc: f.doFunc} } | |
| func (f *fakeGen3Download) Indexd() indexd.IndexdInterface { return &fakeIndexd{doFunc: f.doFunc} } | |
| func (f *fakeGen3Download) Sower() sower.SowerInterface { return nil } |
There was a problem hiding this comment.
download/transfer_test.go:117:41: cannot use fake (variable of type *fakeGen3Download) as g3client.Gen3Interface value in argument to DownloadSingleWithProgress: *fakeGen3Download does not implement g3client.Gen3Interface (missing method Requestor)
download/transfer_test.go:166:41: cannot use fake (variable of type *fakeGen3Download) as g3client.Gen3Interface value in argument to DownloadSingleWithProgress: *fakeGen3Download does not implement g3client.Gen3Interface (missing method Requestor)
If that line is removed, we get this error
Final tweaks from merged PRs
Ran into pre-existing? issues getting unit tests to pass ?