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

update CloudwatchLogs credential object #192

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yzpaul
Copy link

@yzpaul yzpaul commented Jun 22, 2022

allows user provided credentials to work. Prior version was not nested correctly therefore AWS SDK ignored it and used default credentials

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});

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:

  1. only specify region and allow AWS SDK to pull creds from the IAM role
  2. specify the CloudWatchLogsClientConfig directly though awsOptions

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);

Copy link
Owner

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?

Copy link
Author

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.

@yzpaul
Copy link
Author

yzpaul commented Jun 27, 2022

@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?

@lazywithclass
Copy link
Owner

Looks like you solved the problem :D

Do these changes solve the issue for you?

@yzpaul
Copy link
Author

yzpaul commented Jul 1, 2022

@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

@yzpaul
Copy link
Author

yzpaul commented Oct 11, 2022 via email

@lazywithclass
Copy link
Owner

lazywithclass commented Nov 1, 2022

@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.

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.

3 participants