config.Merge behaviors #157

pdalinis · 2015-03-30T21:40:50Z

Been looking at the config and merge functions, and am wondering if they need to be nillable?

aws.DefaultConfig = &aws.Config{DisableSSL: true}
localConfig := &aws.Config{DisableSSL: false}
s := sns.New(localConfig)

expected: DisableSSL should be false
actual: DisableSSL is true

Of course, the use case doesn't seem very valid, but the logic in seems a bit off.

Same issue if you:

aws.DefaultConfig = &aws.Config{MaxRetries: 5}
config := &aws.Config{MaxRetries: aws.DEFAULT_RETRIES}
s := sns.New(localConfig)

expected: MaxRetries == aws.DEFAULT_RETRIES
actual: MaxRetries == 5

The text was updated successfully, but these errors were encountered:

lsegal · 2015-03-30T22:00:41Z

Having nilable config values would make configuration much more complicated than it should be. Realistically you should not be rewriting aws.DefaultConfig, though I can see how this could be a problem if you took an existing config object and ran Merge on it.

I'd prefer to avoid pointer values for primitive types in configuration if there's a way that could be done.

pdalinis · 2015-03-31T15:50:54Z

I'll look into this and get back to you on Friday when I get some free time.

I agree on keeping complexity low.

Switches all primitive values to pointers to ensure configurations can be merged correctly between service instantiation without oddities such as not being able to set a value to its Zero because the Zero value is used to determine if the incoming configuration parameter is set. To facilitate this change and to clarify what aws.String function does it was renamed to aws.StringPtr. This applied to the other pointer helper functions as well. An IntPtr was also added for *int. String -> StringPtr Boolean -> BoolPtr -> IntPtr // New Long -> Int64Ptr Double -> Float64Ptr Time -> TimePtr In addition helper functions were added for each type to return the value that a pointer points to or the type's zero value if the pointer is nil. This saves some of the hassle of checking if a field is nil before dereferencing it in circumstances where nil should be treated as zero. To update to this change the above table of aws.String -> aws.StringPtr function usages need to be updated to their new name aws.String being the most common. Then any usages of aws.Config{} in your code should be updated to wrap the set values with the relevant pointer function. Example: // Code prior to update: svc := s3.New(aws.Config{ Region: "us-west-2", MaxRetries: 2, S3ForcePathStyle: true, } // Prior code read Config.Region value if svc.Config.Region == "us-west-2" {...} // After Updating to this change: svc := s3.New(aws.Config{ Region: aws.StringPtr("us-west-2"), MaxRetries: aws.IntPtr(2), S3ForcePathStyle: aws.BoolPtr(true), } // After this change reading Config.Region value if aws.StringPtrValue(svc.Config.Region) == "us-west-2" {...} Fixes #157

Switches all Config primitive values to custom types to ensure configurations can be merged correctly between service instantiation without oddities such as not being able to set a value to its Zero because the Zero value is used to determine if the incoming configuration parameter is set. The custom types contain the state of if the value was set vs unset. This allows the Config to be merged correctly in cases such as DiableSSL which previous could never be disabled via a Merge(). To facilitate this change and to clarify what aws.String function does it was renamed to aws.StringPtr. This applied to the other pointer helper functions as well. An IntPtr was also added for *int. String -> StringPtr Boolean -> BoolPtr -> IntPtr // New Long -> Int64Ptr Double -> Float64Ptr Time -> TimePtr In addition helper functions were added for each type to return the value that a pointer points to or the type's zero value if the pointer is nil. This saves some of the hassle of checking if a field is nil before dereferencing it in circumstances where nil should be treated as zero. The custom types defined in aws/types.go provide wrappers for string, bool, int, int64, and float64. Each wrapper provides a function to Set() or Get() the wrapped value. The IsSet() method can be used to determine if the value was set at all. Get() will return the type's zero value if the wrapper was not previously set. This does mean also that any place the Config value was read a .Get() needs to be appended to each value's usage. Example: // Code Prior to this update svc := s3.New(&aws.Config{ Region: "us-west-2", MaxRetries: 2, S3ForcePathStyle: true, }) // Prior code read Config.Region value if svc.Config.Region == "us-west-2" {...} // After Updating to this change: svc := s3.New(&aws.Config{ Region: aws.String("us-west-2"), MaxRetries: aws.Int(2), S3ForcePathStyle: aws.Bool(true), }) // After this change reading Config.Region value if svc.Config.Region.Get() == "us-west-2" {...} Fixes #157

jasdel · 2015-06-13T00:12:54Z

HI @pdalinis in #276 i provide two possible methods for addressing the Config.Merg issues. using pointers for all primitive or create custom structs for each primitive. Any feedback you would have would be very helpful. Thanks.

Switches all primitive values to pointers to ensure configurations can be merged correctly between service instantiation without oddities such as not being able to set a value to its Zero because the Zero value is used to determine if the incoming configuration parameter is set. To facilitate this change and to clarify what aws.String function does it was renamed to aws.StringPtr. This applied to the other pointer helper functions as well. An IntPtr was also added for *int. String -> StringPtr Boolean -> BoolPtr -> IntPtr // New Long -> Int64Ptr Double -> Float64Ptr Time -> TimePtr In addition helper functions were added for each type to return the value that a pointer points to or the type's zero value if the pointer is nil. This saves some of the hassle of checking if a field is nil before dereferencing it in circumstances where nil should be treated as zero. To update to this change the above table of aws.String -> aws.StringPtr function usages need to be updated to their new name aws.String being the most common. Then any usages of aws.Config{} in your code should be updated to wrap the set values with the relevant pointer function. Example: // Code prior to update: svc := s3.New(aws.Config{ Region: "us-west-2", MaxRetries: 2, S3ForcePathStyle: true, } // Prior code read Config.Region value if svc.Config.Region == "us-west-2" {...} // After Updating to this change: svc := s3.New(aws.Config{ Region: aws.StringPtr("us-west-2"), MaxRetries: aws.IntPtr(2), S3ForcePathStyle: aws.BoolPtr(true), } // After this change reading Config.Region value if aws.StringPtrValue(svc.Config.Region) == "us-west-2" {...} Fixes #157

Switches all Config primitive values to custom types to ensure configurations can be merged correctly between service instantiation without oddities such as not being able to set a value to its Zero because the Zero value is used to determine if the incoming configuration parameter is set. The custom types contain the state of if the value was set vs unset. This allows the Config to be merged correctly in cases such as DiableSSL which previous could never be disabled via a Merge(). To facilitate this change and to clarify what aws.String function does it was renamed to aws.StringPtr. This applied to the other pointer helper functions as well. An IntPtr was also added for *int. String -> StringPtr Boolean -> BoolPtr -> IntPtr // New Long -> Int64Ptr Double -> Float64Ptr Time -> TimePtr In addition helper functions were added for each type to return the value that a pointer points to or the type's zero value if the pointer is nil. This saves some of the hassle of checking if a field is nil before dereferencing it in circumstances where nil should be treated as zero. The custom types defined in aws/types.go provide wrappers for string, bool, int, int64, and float64. Each wrapper provides a function to Set() or Get() the wrapped value. The IsSet() method can be used to determine if the value was set at all. Get() will return the type's zero value if the wrapper was not previously set. This does mean also that any place the Config value was read a .Get() needs to be appended to each value's usage. Example: // Code Prior to this update svc := s3.New(&aws.Config{ Region: "us-west-2", MaxRetries: 2, S3ForcePathStyle: true, }) // Prior code read Config.Region value if svc.Config.Region == "us-west-2" {...} // After Updating to this change: svc := s3.New(&aws.Config{ Region: aws.String("us-west-2"), MaxRetries: aws.Int(2), S3ForcePathStyle: aws.Bool(true), }) // After this change reading Config.Region value if svc.Config.Region.Get() == "us-west-2" {...} Fixes #157

Switches all primitive values to pointers to ensure configurations can be merged correctly between service instantiation without oddities such as not being able to set a value to its Zero because the Zero value is used to determine if the incoming configuration parameter is set. To facilitate this change and to clarify what aws.String function does it was renamed to aws.StringPtr. This applied to the other pointer helper functions as well. An IntPtr was also added for *int. String -> StringPtr Boolean -> BoolPtr -> IntPtr // New Long -> Int64Ptr Double -> Float64Ptr Time -> TimePtr In addition helper functions were added for each type to return the value that a pointer points to or the type's zero value if the pointer is nil. This saves some of the hassle of checking if a field is nil before dereferencing it in circumstances where nil should be treated as zero. To update to this change the above table of aws.String -> aws.StringPtr function usages need to be updated to their new name aws.String being the most common. Then any usages of aws.Config{} in your code should be updated to wrap the set values with the relevant pointer function. Example: // Code prior to update: svc := s3.New(aws.Config{ Region: "us-west-2", MaxRetries: 2, S3ForcePathStyle: true, } // Prior code read Config.Region value if svc.Config.Region == "us-west-2" {...} // After Updating to this change: svc := s3.New(aws.Config{ Region: aws.StringPtr("us-west-2"), MaxRetries: aws.IntPtr(2), S3ForcePathStyle: aws.BoolPtr(true), } // After this change reading Config.Region value if aws.StringPtrValue(svc.Config.Region) == "us-west-2" {...} Fixes #157

This change refactors the SDK so that Config fields are pointers instead of values. Primiarly this change fixes the bug preventing setting Config fields to the field's zero value when creating new service client instances. ```go svc := s3.New(&aws.Config{ Region: aws.String("us-west-2"), MaxRetries: aws.Int(10), }) ``` Additionally builder pattern `WithX` methods were added to Config. These methods can be chained so inline building of Config objects can be done without using pointers for the fields. ```go svc := s3.New(aws.NewConfig().WithRegion("us-west-2").WithMaxRetries(10)) ``` Fixes #157 Fixes #276 Fixes #124

This change refactors the SDK so that Config fields are pointers instead of values. Primarily this change fixes the bug preventing setting Config fields to the field's zero value when creating new service client instances. ```go svc := s3.New(&aws.Config{ Region: aws.String("us-west-2"), MaxRetries: aws.Int(10), }) ``` Additionally builder pattern `WithX` methods were added to Config. These methods can be chained so inline building of Config objects can be done without using pointers for the fields. ```go svc := s3.New(aws.NewConfig().WithRegion("us-west-2").WithMaxRetries(10)) ``` Fixes #157 Fixes #276 Fixes #124

pdalinis · 2015-07-15T21:30:32Z

@jasdel, I really like using pointers of primitive types. To me, it is more natural and there is less to learn.

I also really like the builder pattern, especially if you have only one or two settings to set.

This change refactors the SDK so that Config fields are pointers instead of values. Primarily this change fixes the bug preventing setting Config fields to the field's zero value when creating new service client instances. ```go svc := s3.New(&aws.Config{ Region: aws.String("us-west-2"), MaxRetries: aws.Int(10), }) ``` Additionally builder pattern `WithX` methods were added to Config. These methods can be chained so inline building of Config objects can be done without using pointers for the fields. ```go svc := s3.New(aws.NewConfig().WithRegion("us-west-2").WithMaxRetries(10)) ``` Fixes #157 Fixes #276 Fixes #124

References: #124, #155, #157, #158, #276, #308

Fixes aws#157 by adding an example for Amazon STS assume role to retrieve credentials.

Breaking Change --- * `service`: Add generated service for wafregional and dynamodbstreams aws#463 * Updates the wafregional and dynamodbstreams API clients to include all API operations, and types that were previously shared between waf and dynamodb API clients respectively. This update ensures that all API clients include all operations and types needed for that client, and shares no types with another client package. * To migrate your applications to use the updated wafregional and dynamodbstreams you'll need to update the package the impacted type is imported from to match the client the type is being used with. * `aws`: Context has been added to EC2Metadata operations.([aws#461](aws/aws-sdk-go-v2#461)) * Also updates utilities that directly or indirectly depend on EC2Metadata client. Signer utilities, credential providers now take in context. * `private/model`: Add utility for validating shape names for structs and enums for the service packages ([aws#471](aws/aws-sdk-go-v2#471)) * Fixes bug which allowed service package structs, enums to start with non alphabetic character * Fixes the incorrect enum types in mediapackage service package, changing enum types __AdTriggersElement, __PeriodTriggersElement to AdTriggersElement, PeriodTriggersElement respectively. * `aws`: Client, Metadata, and Request structures have been refactored to simplify the usage of resolved endpoints ([aws#473](aws/aws-sdk-go-v2#473)) * `aws.Client.Endpoint` struct member has been removed, and `aws.Request.Endpoint` struct member has been added of type `aws.Endpoint` * `aws.Client.Region` structure member has been removed Services --- * Synced the V2 SDK with latest AWS service API definitions. SDK Features --- * `aws`: `PartitionID` has been added to `aws.Endpoint` structure, and is used by the endpoint resolver to indicate which AWS partition an endpoint was resolved for ([aws#473](aws/aws-sdk-go-v2#473)) * `aws/endpoints`: Updated resolvers to populate `PartitionID` for a resolved `aws.Endpoint` ([aws#473](aws/aws-sdk-go-v2#473)) * `service/s3`: Add support for Access Point resources * Adds support for using Access Point resource with Amazon S3 API operation calls. The Access Point resource are identified by an Amazon Resource Name (ARN). * To make operation calls to an S3 Access Point instead of a S3 Bucket, provide the Access Point ARN string as the value of the Bucket parameter. You can create an Access Point for your bucket with the Amazon S3 Control API. The Access Point ARN can be obtained from the S3 Control API. You should avoid building the ARN directly. SDK Enhancements --- * `internal/sdkio`: Adds RingBuffer data structure to the sdk [aws#417](aws/aws-sdk-go-v2#417) * Adds an implementation of RingBuffer data structure which acts as a revolving buffer of a predefined length. The RingBuffer implements io.ReadWriter interface. * Adds unit tests to test the behavior of the ring buffer. * `aws/ec2metadata`: Adds support for EC2Metadata client to use secure tokens provided by the IMDS ([aws#453](aws/aws-sdk-go-v2#453)) * Modifies EC2Metadata client to use request context within its operations ([aws#462](aws/aws-sdk-go-v2#462)) * Reduces the default dialer timeout and response header timeout to help reduce latency for known issues with EC2Metadata client running inside a container * Modifies and adds tests to verify the behavior of the EC2Metadata client. * `service/dynamodb/dynamodbattribute`: Adds clarifying docs on dynamodbattribute.UnixTime ([aws#464](aws/aws-sdk-go-v2#464)) * `example/service/sts/assumeRole`: added sts assume role example ([aws#224](aws/aws-sdk-go-v2#224)) * Fixes [aws#157](aws/aws-sdk-go-v2#157) by adding an example for Amazon STS assume role to retrieve credentials. SDK Bugs --- * `service/dynamodb/dynamodbattribute`: Fixes a panic when decoding into a map with a key string type alias. ([aws#465](aws/aws-sdk-go-v2#465)) * Fixes [aws#410](aws/aws-sdk-go-v2#410), by adding support for keys that are string aliases.

lsegal added enhancement labels Apr 30, 2015

jasdel mentioned this issue Jun 11, 2015

Some Possible Options for fixing aws.Config.Merge #276

Closed

jasdel mentioned this issue Jun 27, 2015

Request Input/Output pointer usage. #294

Closed

lsegal mentioned this issue Jul 14, 2015

Configuring Retries #312

Closed

jasdel mentioned this issue Jul 14, 2015

aws: Config refactor to use pointer for fields [breaking change] #313

Closed

jasdel mentioned this issue Jul 23, 2015

Config Merge refactor and Logger [Breaking Change] #325

Closed

jasdel closed this as completed in 52c7ce7 Jul 28, 2015

jasdel added a commit that referenced this issue Jul 28, 2015

Tag release v0.7.0

67dc9f9

References: #124, #155, #157, #158, #276, #308

lsegal mentioned this issue Sep 2, 2015

aws.String("xxx") ? #363

Closed

diehlaws added feature-request A feature should be added or improved. needs-review This issue or pull request needs review from a core team member. and removed enhancement labels Jan 4, 2019

skotambkar pushed a commit to skotambkar/aws-sdk-go that referenced this issue May 20, 2021

example/service/sts/assumeRole: added sts assume role example (aws#224)

8feaa80

Fixes aws#157 by adding an example for Amazon STS assume role to retrieve credentials.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

config.Merge behaviors #157

config.Merge behaviors #157

pdalinis commented Mar 30, 2015

lsegal commented Mar 30, 2015

Uh oh!

pdalinis commented Mar 31, 2015

Uh oh!

jasdel commented Jun 13, 2015

Uh oh!

pdalinis commented Jul 15, 2015

Uh oh!

config.Merge behaviors #157

config.Merge behaviors #157

Comments

pdalinis commented Mar 30, 2015

lsegal commented Mar 30, 2015

Uh oh!

pdalinis commented Mar 31, 2015

Uh oh!

jasdel commented Jun 13, 2015

Uh oh!

pdalinis commented Jul 15, 2015

Uh oh!