search

codahale / sneaker

A tool for securely storing secrets on S3 using Amazon KMS.
Other
800 stars 34 forks source link

update aws kms field from 'KeyID' to 'KeyId' #11

Closed adelelopez closed 8 years ago

adelelopez commented 8 years ago

do i need to change the readme or the license at all?

codahale commented 8 years ago

No, looks good to me. Thanks for the patch!

snikch commented 8 years ago

Any reason for this change? The current master version of the aws-sdk-go has these capitalised. It's possible this was done as the author had an outdated version of the sdk?

https://github.com/aws/aws-sdk-go/blob/master/aws/credentials/credentials.go#L71

codahale commented 8 years ago

@snikch, I'm not sure what you're trying to point out. This PR isn't related to AWS credentials; it updated the Sneaker code to match changes to the KMS client API. The vendored SDK code was current when the PR was merged.

snikch commented 8 years ago

Sorry, the credentials was just an example of one of the changed files - the third file in this diff is that credentials test file and I just happened to pick that one.

What I was pointing out is that the current master, and in fact the repo at the time of this commit, have all fields suffixed with ID rather than Id, so I'm not sure where this change has come from.

For example, the first file in this pr is Godeps/_workspace/src/github.com/aws/aws-sdk-go/aws/credentials/chain_provider_test.go changing AccessKeyID to AccessKeyId. If you look at the history for this file, it has never had the field with the lower case d. At the time of authoring it looked like this. So I'm not sure how this PR came to be, unless the AWS SDK has had its history rewritten.

To backtrack a bit, the main reason I came here was that I'm using glide to flatten my project's dependencies where I can, and the usage in this library is incompatible with the current AWS library because of this pull request. I figured it might have been a mistake where the OP has accidentally vendored an older version of the library, but after digging deeper I'm not sure how that could be the case either.

Since the current representation is with the upper case ID, would you be averse to me making a PR with the latest version of the aws sdk vendored with Godep - assuming everything compiles correctly?

codahale commented 8 years ago

Ah, that makes sense. Thanks for clarifying! Yes, I’d absolutely take a PR for updating the dependencies. Thank you.

snikch commented 8 years ago

Ok, on further investigation I've discovered what seems to have happened here. Rather than using the 'godep' tool to update the dependencies, a manual update of all instances of KeyID in the Godeps/_workspace directory has occurred.

The AWS sdk actually has a mixture of KeyId and KeyID. The kms package uses KeyId, which I believe is why this change is initiated, but unfortunately it changed some files so they are not a match with any version of the actual sdk.

I'll create a PR with the current Godeps revendored.