-
Notifications
You must be signed in to change notification settings - Fork 105
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
update CloudwatchLogs credential object #192
base: master
Are you sure you want to change the base?
Conversation
allows user provided credentials to work. Prior version was not nested correctly therefore AWS SDK ignored it
index.js
Outdated
@@ -49,7 +49,7 @@ var WinstonCloudWatch = function(options) { | |||
config = assign(config, options.awsOptions); | |||
} | |||
|
|||
this.cloudwatchlogs = new CloudWatchLogs(config); | |||
this.cloudwatchlogs = new CloudWatchLogs({credentials:config}); |
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.
How are you instantiating the WinstonCloudWatch
transport? When I do so via:
new WinstonCloudWatch({
...,
awsAccessKeyId,
awsSecretKey,
awsRegion,
})
This fix doesn't work. Specifically because in CloudWatchLogsClientConfig
, region
is specified at the same level of credentials
.
Additionally, I believe this proposed fix would break the transport for those who:
- only specify region and allow AWS SDK to pull creds from the IAM role
- specify the
CloudWatchLogsClientConfig
directly thoughawsOptions
Thus, I would propose something like changing line 40 to:
config = {
credentials: {
accessKeyId: awsAccessKeyId,
secretAccessKey: awsSecretKey,
},
region: awsRegion
};
And this line back to its original
this.cloudwatchlogs = new CloudWatchLogs(config);
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 with @nikhilrajaram, what do you think @yzpaul?
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.
@lazywithclass yes @nikhilrajaram is right according to the docs, I didn't notice that issue because all of my test cases were in the same region.
@lazywithclass I'm fairly new to GitHub, should I incorporate his proposed change and do another pull request? How is this supposed to work so everything stays clean? |
Looks like you solved the problem :D Do these changes solve the issue for you? |
@lazywithclass yep, works for me, although I haven't set up an account in another region to confirm it, this seems right according to the docs |
@nikhilrajaram <https://github.com/nikhilrajaram> is absolutely correct, I
never noticed the region issue because in my testing all of my test cases
were in the same region.
…On Mon, Jun 27, 2022, 14:04 Alberto Zaccagni ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In index.js
<#192 (comment)>
:
> @@ -49,7 +49,7 @@ var WinstonCloudWatch = function(options) {
config = assign(config, options.awsOptions);
}
- this.cloudwatchlogs = new CloudWatchLogs(config);
+ this.cloudwatchlogs = new CloudWatchLogs({credentials:config});
I agree with @nikhilrajaram <https://github.com/nikhilrajaram>, what do
you think @yzpaul <https://github.com/yzpaul>?
—
Reply to this email directly, view it on GitHub
<#192 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKDOOXXCLCGCLPMFCOWAETVRHUKDANCNFSM5ZOOYUQA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@yzpaul sorry I am confused by your last comment, is the region issue solved by your last push? If so I will test this in different regions and then publish a new version. |
allows user provided credentials to work. Prior version was not nested correctly therefore AWS SDK ignored it and used default credentials