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

fix: actually log the userinfo object returned by the IAM #76

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

Conversation

bushjames
Copy link
Contributor

@bushjames bushjames commented Aug 17, 2021

Userinfo object is not currently logged due to a misunderstanding of the logger api. This fixes it.

Example of current log output showing the issue:

{"level":30,"time":1629199407789,"pi[redacted]XFiucU"}},"msg":"Got userinfo:"}

Userinfo object is not currently logged due to a misunderstanding of the logger api. This fixes it.
@bushjames bushjames changed the title bugfix: actually log the userinfo object returned by the IAM fix: actually log the userinfo object returned by the IAM Aug 17, 2021
Copy link
Contributor

@partiallyordered partiallyordered left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of other instances of this type of usage of the logger later in the same file, perhaps change those now also?

Also, probably want to bump the version. I discovered npm version patch the other day. Run that command in the directory containing the package.json file. There's also npm version minor and npm version major.

Tangentially, this pattern with multiple arguments to the logger methods (that does not work since the logger changed to pino) might exist more widely in this code. could be a good application of codemods.

@@ -14,7 +14,7 @@ const permit = async (userInfoURL, token, requestMethod, requestPath, log) => {
agent: selfSignedAgent,
};
const userinfo = await fetch(userInfoURL, opts).then((res) => res.json());
log.info('Got userinfo:', userinfo);
log.info(`Got userinfo: ${userinfo}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will result in better structure in the logs (although it's less logger-agnostic).

Suggested change
log.info(`Got userinfo: ${userinfo}`);
log.child({ userInfo }).info('Got userinfo');

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.

2 participants