feat(log): bump log module from 1.4.0 to 6.3.1

[joeyguerra]

Intent

Update to the latest version

Approach

Sinon couldn't wrap the warning function in the log module when I updated log to the latest version. So I created an object to decorate the underlying log module.

This means that technically the logger API has changed.

Reasoning

I didn't want to change the tests because, to me, they represent the agreed upon public API. So my thought is that if the tests pass, the probability of breaking a dependent is low.

1492

[levenleven]

👋 if we're already replacing a logger module would be nice to go with something that supports structured logging like https://www.npmjs.com/package/pino

[joeyguerra]

So. I’m already questioning myself suggesting replacing logging with this PR. Questions like:

Is now the time to make such a big move? And not at least bump to a minor version release?

is this even a big move?

What minimum version of Node.is does this support?

what minimum version should Hubot support?

I’m unfamiliar with pino, but structured logging seems like a common requirement “nowadays”.

what are you’re thoughts?

[xurizaemon]

Lots of change has come in lately which is good, but too much can be a problem.

PR description currently doesn't say why were making this change, it describes a problem which occurred. I think "Sinon couldn't wrap the warning function in the log module when I updated log to the latest version." means "some test coverage stopped working", which might suggest replacing the logger (but are there other options like fixing tests? it helps to document why not that approach).

Considering a structured logger is also a good idea and that would be a feature change I think, since it would change one of Hubot's interfaces (for the better). Again I'd like to see the why of it in the issue/PR rather than just the change proposed. Tedious, I know :)

Sidebar: If we had semantic-release configured (#965?) it would put onus on us to consider the "chore" commit message here (docs), since "chore" says "nothing to see here" and this is a feature version change IMO?

[joeyguerra]

Apologies.

Great points. I'll update the title and description to include the why and add my thought process.

I've reverted the change back to "just update the log version" and try an approach that will pass the tests so we can consider moving forward with just that change and create a different PR to discuss replacing log with Pino for the structured logging aspects.

My thought about chore vs feature is that the tests represent the public API and if they pass and the code used is supported in Node.js v10+, then the probability that the API has not changed, is high.

[levenleven]

Yeah, all above makes sense. Sorry for bringing the discussion into PR.

One thing to consider is that newer version of the same log package can also have breaking changes and can break stuff on users' side.

what minimum node version should Hubot support?

I think we should align with https://endoflife.date/nodejs, so currently it would be 16+ (of course we should bump major when we change this requirement)

[xurizaemon]

All good @levenleven - thanks for proposing!

@joeyguerra yep if it's just upgrading existing logging component then chore is probably right; if we swapped loggers and Hubot's log output changed, that might impact folks who are parsing it, and therefore I'd argue it's worth some more significant signifier. Which is what I was trying to say - that once those commits have "real world impact" (ie firing semantic-release), it puts the requirement on us to consider and describe what we changed, which is hopefully for the better.

API is one primary interface of Hubot, but log output is also (IMO) an interface (albeit secondary), and chat responses are another primary interface, etc.

semantic-release is all very theoretical until we wire it up, I only mentioned it as it might "nudge" how we describe changes. I'm assuming semantic-release was ever wired up for real, I only saw it in the docs!

[joeyguerra]

Btw, I believe a PR's main purpose to have these discussions ;)

[joeyguerra]

API is one primary interface of Hubot, but log output is also (IMO) an interface (albeit secondary), and chat responses are another primary interface, etc.

Based on this thought, "log output is also an interface", I'm going to change the PR title to feat(log) because I can't say for sure that the log output has NOT changed. I also feel like since we're going from a 1.x to a 3.x version of log, that the probability the output has changed is high.

Therefore, I'd like to get the other PRs merged first so that we can do another release. Then consider merging this one after that for a more impactful release.

[xurizaemon]

Gosh, "feat" feels like a lot now! Sorry 😄 but all good. We are surely going to hit a feat release with all the change that's come in anyway.

@joeyguerra just want to check you realise that -

1. 👻 semantic-release uses commit messages not issue titles to determine the significance of changes in a new release, and
2. you can change existing commit messages using git rebase or git commit --amend

(I promised somewhere to show you how that was done, and haven't yet done this.)

EDIT: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/changing-a-commit-message looks alright for that. Github says "we strongly discourage force pushing" and I think that's a good simplification but in a PR branch like this, that's exactly what you'd do to change the commits. (I think of it like sudo: a tool to be used only when appropriate, and with consideration each time.)

[joeyguerra]

@xurizaemon I'm trying to figure out how to resolve your review. I made the changes so it should be good.

[xurizaemon]

Weird, sorry if that left it blocked! It looked like the relevant changes were made but maybe needed further approval after that?

[joeyguerra]

No worries. Yes, I think as a reviewer, you have to go back and click “approved” somewhere. Thank you. Gonna smerge.

[joeyguerra]

Welp. No I’m not. I had to remove node 14 from the action but this PR page thinks it’s still a check. Perhaps it’s a cache invalidation problem?

[joeyguerra]

Updated: Nevermind. The problem was the package-lock file version. I executed npm install with 14.21.3, which updated the package-lock file to a version it understands and then added version 14.x back into the strategy matrix. Which worked.

@levenleven @technicalpickles I need help with merging this PR. I removed building with node_version: 14.x in this PR because the build is failing for node version 14 with

npm ci
npm WARN prepare removing existing node_modules/ before installation
npm ERR! Cannot read property 'async' of undefined

Since we're trying to follow the Node.js release cycle, removing building with 14 seems like the right direction. But now that check is still expected but won't pass because it's essentially been removed from the workflow file.

Any suggestions on how to move forward?