Skip to content

update CloudwatchLogs credential object#192

Open
yzpaul wants to merge 3 commits into
lazywithclass:masterfrom
yzpaul:master
Open

update CloudwatchLogs credential object#192
yzpaul wants to merge 3 commits into
lazywithclass:masterfrom
yzpaul:master

Conversation

@yzpaul

@yzpaul yzpaul commented Jun 22, 2022

Copy link
Copy Markdown

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
Comment thread index.js Outdated
}

this.cloudwatchlogs = new CloudWatchLogs(config);
this.cloudwatchlogs = new CloudWatchLogs({credentials:config});

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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

yzpaul commented Jun 27, 2022

Copy link
Copy Markdown
Author

@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
Copy Markdown
Owner

Looks like you solved the problem :D

Do these changes solve the issue for you?

@yzpaul

yzpaul commented Jul 1, 2022

Copy link
Copy Markdown
Author

@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

yzpaul commented Oct 11, 2022 via email

Copy link
Copy Markdown
Author

@lazywithclass

lazywithclass commented Nov 1, 2022

Copy link
Copy Markdown
Owner

@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