-
Notifications
You must be signed in to change notification settings - Fork 118
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
Replace console.log with structured logging #508
base: main
Are you sure you want to change the base?
Conversation
…gger.info/logger.error
|
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
✅ Deploy Preview for endearing-brigadeiros-63f9d0 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@LewisBroadhurst - thanks for the pull request - you're a star ⭐ In terms of keeping the old |
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.
Looking good! 🍰
Can we think about whether we should use winston
logging in log()
in Step.js
:
log(message) {
const m = `${this.stepName} - ${message}`;
this.logs.push(m);
console.info(m);
}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #508 +/- ##
==========================================
+ Coverage 56.96% 57.97% +1.01%
==========================================
Files 46 40 -6
Lines 1566 1078 -488
==========================================
- Hits 892 625 -267
+ Misses 674 453 -221 ☔ View full report in Codecov by Sentry. |
I think this is an example of keeping the console.log I mentioned initially, as having the values logged to console will be a much better UX? (and an npx installer wouldn't have log access?) For this would the best solution be adding the winston logging in to the step, as well as keeping the console.log? Then we get the UX positives and logging. Can remove the double logging from checkRepoInAuthorisedList.js with this too 😄 step.log(`repo ${action.repo} is not in the authorisedList, ending`);
- logger.info('setting error');
step.setError(`Rejecting repo ${action.repo} not in the authorisedList`);
- logger.error(`Rejecting repo ${action.repo} not in the authorisedList`); + log(message, isError = false) {
const m = `${this.stepName} - ${message}`;
this.logs.push(m);
+ if (!isError) {
+ console.info(m);
+ logger.info(m);
+ } else {
+ console.error(m);
+ logger.error(m);
+ } |
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.
Small suggestion on winston setup.
…st/git-proxy into structured-logging
@LewisBroadhurst are you able to address the conflicts? |
Closes #399
Winston logger has been used to replace the console.log() node logging in the git-proxy application. Logs generated from git-proxy use now have a format of
[timestamp] [log level]: [log message]
, and can be found in the filesgit-proxy.log
anderror.log
under thesrc/logging
directory.Before merging, I wanted to confirm that the old console.log() lines should be removed also. Maybe they are useful in some cases?