-
Couldn't load subscription status.
- Fork 27
Support for .NETFramework 4.5 and .NETStandard 1.3 #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply. I've been busy lately. This looks mostly good. There are just a few things to resolve.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't need to expose this since it's only necessary to provide it when using instance profile credentials. Instance profile credentials will be used when no access key or sercret are provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually needed when we use role credentials. Storing credentials in config files is discouraged in favour of using roles, and to follow that route we will need the token. That is exactly my case by the way - I run my app in a docker container which is registered in an ECS task. The task is assigned a role which is different from the instance's role and has all the necessary permissions. I extract credentials from my task role and have to pass them along with the token to be able to use them. There are other scenarios when we might need to pass the token. For example, when we want to work with assumed roles or want to use temporary (session) credentials derived from other credentials. In fact, I was thinking that since the user knows best what credentials they want to use including the token, they should always pass all authentication data to the library. Or we can call internally Amazon.Runtime.FallbackCredentialsFactory.GetCredentials() which will create dependency on AWSSDK.Core, but it's still better than always assuming that we should get the instance role. These further changes are perhaps subject of a longer conversation, so let's leave it as it is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that makes sense then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be internal. I'd rather expose only what the consumer needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal
davemcdermid
commented
Dec 5, 2016
Hey, what's the status of this pull-req? Been testing locally, would love to have access to a pre-release on nuget.org :)
Thanks
@davemcdermid Looks good. I just need to test it on my end before I push a release. I'll try to get that done soon.
@davemcdermid @nanestev nuget released as 2.2.0
davemcdermid
commented
Dec 5, 2016
Fantastic, thank you!
you're welcome
Uh oh!
There was an error while loading. Please reload this page.
Support for net45 comes at the price of a couple compiler directives because ElasticSearch.Net for net45 has different API. Also I needed to create abstraction of request and header collection to be able to reuse the original code with both HttpRequestMessage/WebHeaderCollection and HttpWebRequest/HttpRequestHeaders and all the differences that come with them. Very few other changes of the original code like ditching JavaScriptSerializer in favor of JSON.Net. Unit tests run for both netcore and net451 (because NUnit runner is targetting net451).
The signing key was not present so had to switch off signing, but you can always switch it back on.
Also I added Token property to the AwsSettings class. It is needed, because we can't use role credentials without it (for example when working with assumed roles or ECS task roles).
Cheers!
N