chore(blobfs): Remove obsolete overrides for s3 blob opener#1173
chore(blobfs): Remove obsolete overrides for s3 blob opener#1173hairyhenderson wants to merge 1 commit intomainfrom
Conversation
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
There was a problem hiding this comment.
Benchmark
Details
| Benchmark suite | Current: 070c9d1 | Previous: e0bddf7 | Ratio |
|---|---|---|---|
BenchmarkPathForDirFS |
92.7 ns/op 40 B/op 2 allocs/op |
101.9 ns/op 40 B/op 2 allocs/op |
0.91 |
BenchmarkPathForDirFS - ns/op |
92.7 ns/op |
101.9 ns/op |
0.91 |
BenchmarkPathForDirFS - B/op |
40 B/op |
40 B/op |
1 |
BenchmarkPathForDirFS - allocs/op |
2 allocs/op |
2 allocs/op |
1 |
BenchmarkSplitRepoPath/0 |
11.84 ns/op 0 B/op 0 allocs/op |
12.6 ns/op 0 B/op 0 allocs/op |
0.94 |
BenchmarkSplitRepoPath/0 - ns/op |
11.84 ns/op |
12.6 ns/op |
0.94 |
BenchmarkSplitRepoPath/0 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkSplitRepoPath/0 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkSplitRepoPath/1 |
15.26 ns/op 0 B/op 0 allocs/op |
17.4 ns/op 0 B/op 0 allocs/op |
0.88 |
BenchmarkSplitRepoPath/1 - ns/op |
15.26 ns/op |
17.4 ns/op |
0.88 |
BenchmarkSplitRepoPath/1 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkSplitRepoPath/1 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkSplitRepoPath/2 |
36.2 ns/op 4 B/op 1 allocs/op |
37.28 ns/op 4 B/op 1 allocs/op |
0.97 |
BenchmarkSplitRepoPath/2 - ns/op |
36.2 ns/op |
37.28 ns/op |
0.97 |
BenchmarkSplitRepoPath/2 - B/op |
4 B/op |
4 B/op |
1 |
BenchmarkSplitRepoPath/2 - allocs/op |
1 allocs/op |
1 allocs/op |
1 |
BenchmarkSplitRepoPath/3 |
37.93 ns/op 8 B/op 1 allocs/op |
39 ns/op 8 B/op 1 allocs/op |
0.97 |
BenchmarkSplitRepoPath/3 - ns/op |
37.93 ns/op |
39 ns/op |
0.97 |
BenchmarkSplitRepoPath/3 - B/op |
8 B/op |
8 B/op |
1 |
BenchmarkSplitRepoPath/3 - allocs/op |
1 allocs/op |
1 allocs/op |
1 |
BenchmarkSplitRepoPath/4 |
36.96 ns/op 8 B/op 1 allocs/op |
36.6 ns/op 8 B/op 1 allocs/op |
1.01 |
BenchmarkSplitRepoPath/4 - ns/op |
36.96 ns/op |
36.6 ns/op |
1.01 |
BenchmarkSplitRepoPath/4 - B/op |
8 B/op |
8 B/op |
1 |
BenchmarkSplitRepoPath/4 - allocs/op |
1 allocs/op |
1 allocs/op |
1 |
BenchmarkSplitRepoPath/5 |
40.59 ns/op 4 B/op 1 allocs/op |
42.62 ns/op 4 B/op 1 allocs/op |
0.95 |
BenchmarkSplitRepoPath/5 - ns/op |
40.59 ns/op |
42.62 ns/op |
0.95 |
BenchmarkSplitRepoPath/5 - B/op |
4 B/op |
4 B/op |
1 |
BenchmarkSplitRepoPath/5 - allocs/op |
1 allocs/op |
1 allocs/op |
1 |
BenchmarkSplitRepoPath/6 |
15.92 ns/op 0 B/op 0 allocs/op |
17.13 ns/op 0 B/op 0 allocs/op |
0.93 |
BenchmarkSplitRepoPath/6 - ns/op |
15.92 ns/op |
17.13 ns/op |
0.93 |
BenchmarkSplitRepoPath/6 - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkSplitRepoPath/6 - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Pull request overview
This PR removes obsolete custom S3 URL opener implementation and delegates to the Go CDK's built-in s3blob.URLOpener with UseV2 enabled. The custom implementation was originally created to work around limitations in the Go CDK (see issue #3512), but is no longer needed as the Go CDK now supports the required functionality.
Changes:
- Refactored fake IMDS server test helper into a separate package with cleaner API
- Replaced custom S3 URL opener with a thin wrapper that adds IMDS region lookup functionality
- Updated parameter handling to support both
s3ForcePathStyle(for backward compatibility) anduse_path_style
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/tests/fakeimds/fakeimds.go | Renamed package from awsimdsfs to fakeimds, made Server function public, and improved API to return both server and URL |
| blobfs/s3opener.go | Removed ~250 lines of custom S3 URL opener implementation, replaced with thin wrapper that only handles IMDS region lookup |
| blobfs/s3blob.go | Removed translateV1Params function; now keeps s3ForcePathStyle as-is for backward compatibility while disableSSL is still translated to disable_https |
| blobfs/blob.go | Updated to use new s3WithRegionOpener wrapper with Go CDK's s3blob.URLOpener |
| blobfs/blob_test.go | Enhanced tests with better organization, added test case for IMDS region lookup, updated URL parameters to use documented names |
| awsimdsfs/awsimdsfs_test.go | Updated to use new fakeimds.Server API |
Comments suppressed due to low confidence (1)
internal/tests/fakeimds/fakeimds.go:399
- The error from url.Parse is being silently ignored. While srv.URL should always be valid in this test helper context, it would be better to handle the error explicitly or add a comment explaining why it's safe to ignore. Consider either checking the error and calling t.Fatal if it occurs, or adding a comment explaining that httptest.Server.URL is always a valid URL.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // not relevant for read operations, but are be passed through to the | ||
| // Go CDK | ||
| case "kmskeyid", "ssetype": | ||
| // not relevant for read operations, but are be passed through to the |
There was a problem hiding this comment.
Grammatical error in comment. "are be passed" should be "are passed".
| // not relevant for read operations, but are be passed through to the | |
| // not relevant for read operations, but are passed through to the |
|
This pull request is stale because it has been open for 60 days with If it's still relevant, one of the following will remove the stale
|
No description provided.