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 debug logging of undefined query params #8540

Merged
merged 2 commits into from Dec 28, 2022

Conversation

chris48s
Copy link
Member

If we pass { someparam: undefined } to got, someparam will not be passed at all in the querystring. When we log the params to console (for example when running npm run badge), this currently appears as ?someparam=undefined. This PR switches us to dropping these params completely, in line with the behaviour of got.

@chris48s chris48s added the core Server, BaseService, GitHub auth label Oct 17, 2022
@shields-ci
Copy link

shields-ci commented Oct 17, 2022

Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against a1a1a43

@calebcartwright
Copy link
Member

Not necessarily opposed to this, but curious as to how feasible/preferable it might be to adjust this in our logging (less common) vs. what I imagine is essentially duplicative processing of got's internal behavior that's happening at runtime for every request

@chris48s
Copy link
Member Author

We could define a custom version of logTrace just for when we log the request URL.

It has been a while since I looked at this but I think the reason why I thought stripping the undefined values and then passing the modified params object to both got and logTrace is this way we are passing the exact same object to both, rather than logging a slightly different thing than what we passed got. There's not a lot in it though.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Perhaps a bit overly cautious from my end but I'd be inclined to deploy this in isolation

@chris48s
Copy link
Member Author

Yeah good call. I'll leave this one to mature for a bit 🧀

@chris48s chris48s merged commit 00f35c6 into badges:master Dec 28, 2022
@chris48s
Copy link
Member Author

I've just pushed this to prod. I'll keep an eye on sentry/issue tracker over the next few days and plan to hold off doing another deploy until the weekend

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

Successfully merging this pull request may close these issues.

None yet

3 participants