chore: Upgrade to aws-sdk-go-v2 #481
Conversation
This is a major update to chamber's support for the S3, SSM, and Secrets Manager store implementations. Every effort was made to preserve functionality, but there is one gap. The v2 SDK does not expose a retryer field for a minimum throttle delay, so that argument is currently ignored when constructing new SSM stores. Support for the delay will be addressed later. The v2 SDK does not offer "iface" interfaces for the various clients, so instead interfaces tailored to what chamber uses are defined. For testing, these new interfaces are mocked, and mock types are generated using github.com/matryer/moq. You don't need moq to use chamber or even to build it, but only if you are developing chamber and make a change to an API interface. Also, old code in the SSM store implementation that allowed it to work without IAM permissions for ssm:GetParametersByPath has been eliminated. The permissions have been expected for a long time now.
bhavanki
requested review from
mckern,
cdignam-segment and
a team
and removed request for
a team
|
FWIW, I've been running with this version of chamber for a week and I use it pretty often. |
| @@ -32,7 +43,7 @@ dist/: | |||
|
|
|||
| build: chamber | |||
|
|
|||
| chamber: | |||
| chamber: $(SRC) | |||
There was a problem hiding this comment.
Double-checking intent: a change to any .go file in the repository will now mark the chamber target as out of date?
There was a problem hiding this comment.
That's the idea. I'm not wedded to this change, so if you have concerns, I'm happy to drop it.
| @@ -265,28 +273,33 @@ func (s *SSMStore) ListServices(service string, includeSecretName bool) ([]strin | |||
|
|
|||
| if s.usePaths { | |||
| describeParametersInput = &ssm.DescribeParametersInput{ | |||
| MaxResults: aws.Int64(50), | |||
| ParameterFilters: []*ssm.ParameterStringFilter{ | |||
| MaxResults: aws.Int32(50), | |||
There was a problem hiding this comment.
functional change or stylistic change?
There was a problem hiding this comment.
The v2 SDK changes the field type.
|
What're you thinking about for a version number on this? Clearly not a patch release but I'm not sure it's a major. Minor? |
Co-authored-by: Ryan McKern <344926+mckern@users.noreply.github.com>
At least minor, for sure. But, I was thinking we could use this disruption as an opportunity to remove some deprecated features. For example, in this commit I removed some old workaround code for a IAM permission that became required in the distant past. Those removals would count as breakage, meriting a major release. |
Ah, if a major version is on the table then do we want to talk about what else might go into it and hold off releasing after this is merged? |
Definitely! There's at least one other thing I recall spotting that could be dropped, I forget what it is at the moment. Also, there are some improvements that can follow from this PR, like how contexts are handled (oh, and figuring out what to do about the minimum throttle delay setting). I don't see any problem letting this update sit unreleased on master while we do some spring cleaning. |
This is a major update to chamber's support for the S3, SSM, and Secrets Manager store implementations. Every effort was made to preserve functionality, but there is one gap.
The v2 SDK does not expose a retryer field for a minimum throttle delay, so that argument is currently ignored when constructing new SSM stores. Support for the delay will be addressed later.
The v2 SDK does not offer "iface" interfaces for the various clients, so instead interfaces tailored to what chamber uses are defined. For testing, these new interfaces are mocked, and mock types are generated using github.com/matryer/moq. You don't need moq to use chamber or even to build it, but only if you are developing chamber and make a change to an API interface.
Also, old code in the SSM store implementation that allowed it to work without IAM permissions for ssm:GetParametersByPath has been eliminated. The permissions have been expected for a long time now.
Resolves #478.