tanmayrauth commented on code in PR #1506:
URL: https://github.com/apache/iceberg-go/pull/1506#discussion_r3633826960
##########
io/gocloud/s3.go:
##########
@@ -167,18 +169,35 @@ func resolveUsePathStyle(endpoint string, props
map[string]string) bool {
return usePathStyle
}
-func createS3Bucket(ctx context.Context, parsed *url.URL, props
map[string]string) (*blob.Bucket, error) {
+// resolveS3AWSConfig returns the AWS config for the S3 FileIO, preferring an
+// ambient context config but letting explicit s3.* credentials override it.
+func resolveS3AWSConfig(ctx context.Context, props map[string]string)
(*aws.Config, error) {
var (
awscfg *aws.Config
err error
)
if v := utils.GetAwsConfig(ctx); v != nil {
awscfg = v
- } else {
- awscfg, err = ParseAWSConfig(ctx, props)
- if err != nil {
- return nil, err
- }
+ } else if awscfg, err = ParseAWSConfig(ctx, props); err != nil {
+ return nil, err
+ }
+
+ // Copy before overriding so the shared context config stays untouched.
+ if props[io.S3AccessKeyID] != "" || props[io.S3SecretAccessKey] != "" {
+ cfg := *awscfg
Review Comment:
since you're introducing the copy-to-avoid-mutation pattern here, you could
hoist it so it also covers the `awscfg.HTTPClient = newS3BuildableClient()`
assignment in createS3Bucket. Today that copy only happens on the props-creds
branch, so in the no-props branch the HTTPClient assignment still mutates
the shared context config (pre-existing, and only when it's nil, so low
impact) — hoisting the copy would fully realize the "don't mutate the shared
config" intent. Not a blocker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
