Skip to content
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

retrieve detected security fields from readCredentials #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guss77
Copy link

@guss77 guss77 commented Nov 2, 2014

There is something weird going on here, but basically readRecommendation() return data in the object passed as the 2nd parameter in the callback, but because it returns a reference to the same object passed as its 1st parameter, then it also updates that. What it doesn't update is the genericAWSClient() local variables inited in lines 43~50 - which is what the rest of the code actually use. so instead of ignoring data found by readRecommendations() we should use it.

I would have refactored the API of readRecommendations to be less confusing (both have an "out parameter" and pass it to the callback? weird) but this change is the minimal to get IAM role based authentication to work.

There is something weird going on here, but basically `readRecommendation()` return data in the object passed as the 2nd parameter in the callback, but because it returns a reference to the same object passed as its 1st parameter, then it also updates that. What it doesn't update is the `genericAWSClient()` local variables inited in lines 43~50 - which is what the rest of the code actually use. so instead of ignoring data found by `readRecommendations()` we should use it.

I would have refactored the API of `readRecommendations` to be less confusing (both have an "out parameter" and pass it to the callback? weird) but this change is the minimal to get IAM role based authentication to work.
@guss77
Copy link
Author

guss77 commented Nov 2, 2014

This is a fix to issue #89

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant