chore(deps): bump minimum peer dependency bound with semantic-release

[babblebey]

This PR Bumps the minimum bound of the semantic-release peer dependency in order to allow the plugin to consume the new warn function added in semantic-release/semantic-release#3423

BREAKING CHANGE: the minimum required version of semantic-release to use @semantic-release/github is now v24.1.0

Related Issue

Resolves #902
Fixes #920

[travi]

i would recommend including the change that is prompting this breaking change in this PR. that way the reasoning for the breaking change is more clear in the release. i would keep the breaking change as a separate commit from the other changes. that way we can merge without squashing and the release notes generated by semantic-release will list the breaking change for the peer dependency bump as well as the change(s) for the use of the new feature from core.

[babblebey]

i would recommend including the change that is prompting this breaking change in this PR. that way the reasoning for the breaking change is more clear in the release.

Yea, this I did with 85d0ad0 and 0843ffe but the PR was failing CI with the error below in some unit tests....

TypeError {
  message: 'logger.warn is not a function',
}

...hence I thought to revert.

that way we can merge without squashing and the release notes generated by semantic-release will list the breaking change for the peer dependency bump as well as the change(s) for the use of the new feature from core.

This looks good YES, but does that will mean we do a "Merge Commit"?? If so, maybe it will be better to just redo this PR in-order to avoid the "Revert" commits showing up in release notes and on master branch commit history (this is assuming we're good to allow the PR fly regardless of the failing unit test in CI) 🤔

What'd you think?

[travi]

Yea, this I did with 85d0ad0 and 0843ffe but the PR was failing CI with the error below in some unit tests

the good news about this is that the tests were failing for a good reason. they were highlighting that changing the peer dependency was not enough for the new usage of the new logger function to work properly. keep in mind that a peer-dependency definition is only declaring the range that is considered to be compatible with the intended usage. to satisfy the peer-dependency requirement, we also need to actually depend on a compatible version. we take care of that requirement by capturing a dev-dependency on semantic-release for usage in the tests. for that to work in your updated usage and related tests, that dev-dependency declaration also needs to be updated to be compatible with the new peer-dependency update (this would be a chore type change since it does not impact consumers).

does that will mean we do a "Merge Commit"??

yes. personally, i tend to prefer normal merges most of the time because it allows us to tell more of the story than a single commit does. semantic-release does a great job of communicating the details when it has more commits to work with.

maybe it will be better to just redo this PR in-order to avoid the "Revert" commits showing up in release notes and on master branch commit history

we can worry about this part once we are ready to merge. lets get the changes to the point of everything passing and have the final changes that we want.

after that, we can decide what story we want to tell with the steps in the commits. honestly, i dont think it is all that bad to include the reverts, but if we want to clean those up, we can. starting over as a new PR is an option, but git also has great flexibility to modify the history of a branch, so we could choose to take the steps to clean up the story in the existing branch that is tied to this PR. i think there is value in learning those capabilities of git at some point in anyone's learning journey, and i'm happy to help if youre interested as part of this. however, if that sounds too advanced for the goals of this PR, a separate PR to replay the steps that we confirm in the first attempt is totally acceptable as well.

[travi]

the good news about this is that the tests were failing for a good reason. they were highlighting that changing the peer dependency was not enough for the new usage of the new logger function to work properly. keep in mind that a peer-dependency definition is only declaring the range that is considered to be compatible with the intended usage. to satisfy the peer-dependency requirement, we also need to actually depend on a compatible version. we take care of that requirement by capturing a dev-dependency on semantic-release for usage in the tests.

i looked at this a little more closely and realized the the devDependency was already updated, likely by renovate. it confused me since that change wasnt part of this PR. so, that leaves me unsure about why specifically your tests failed, but it still comes down to the error you shared:

TypeError {
message: 'logger.warn is not a function',
}

that is still highlighting that the logger function you are attempting to use is not available in that context for some reason. we need that to work to give us more confidence that it will work after releasing the change in a real context. it is worth investigating to understand what is causing that failure and resolving it as part of this change

[babblebey]

yes. personally, i tend to prefer normal merges most of the time because it allows us to tell more of the story than a single commit does. semantic-release does a great job of communicating the details when it has more commits to work with.

I totally agree with how semantic-release handles commit, the more commits the richer the details in the release note; better than just a single line change in the release note.

but git also has great flexibility to modify the history of a branch, so we could choose to take the steps to clean up the story in the existing branch that is tied to this PR. i think there is value in learning those capabilities of git at some point in anyone's learning journey, and i'm happy to help if youre interested as part of this.

I am very interested in this for sure 😃

we need that to work to give us more confidence that it will work after releasing the change in a real context. it is worth investigating to understand what is causing that failure and resolving it as part of this change

Yea, I'll do some digs

[babblebey]

Yea, I'll do some digs

...and It didn't take any longer than 40secs to figure that I was missing the mock warn method in the test context.logger, I mean I figured the logger function within the test context isn't even real in the first place.... soo. 😆

so we could choose to take the steps to clean up the story in the existing branch that is tied to this PR..

And I figured this out too 😉, guess I just needed to know it was possible, thanks for that 😁

[travi]

And I figured this out too 😉, guess I just needed to know it was possible, thanks for that 😁

awesome! great work!

might need to do it one more time, though. since we'll be doing a normal merge rather than a squash, we need to make sure that we include the BREAKING CHANGE: footer in the breaking commit(s) so that we end up communicating the breaking nature of the release with a major version bump. i think this is the commit that will need that: b7577aa (#916)

I was missing the mock warn method in the test context.logger

ah, that makes sense. quick fix for that situation, then. unfortunately, that doesnt add a lot of confidence that the combination of the core update and the usage in this plugin integrate successfully, just because the integration is mocked. do you happen to be aware whether that is exercised by the integration test? this is pretty low risk, and i see no reason to expect it not to work, but knowing that it is executed somewhere without resulting in an error would help some with confidence.

[travi]

one other thing to consider before we merge this... since we attempt to avoid unnecessary breaking changes too often, knowing that we will be releasing a major version here makes it a good opportunity to consider whether there are any other breaking changes that we should group with this. nothing is coming to my mind, but worth considering in case anything more comes to mind (that wouldnt require significant investment)

edit: defining a support policy for GHES could be something worth considering

[babblebey]

we need to make sure that we include the BREAKING CHANGE: footer in the breaking commit(s) so that we end up communicating the breaking nature of the release with a major version bump

Yea, will do... gotta be the case since we're merging commits 😉

just because the integration is mocked. do you happen to be aware whether that is exercised by the integration test? this is pretty low risk, and i see no reason to expect it not to work, but knowing that it is executed somewhere without resulting in an error would help some with confidence.

Yea, it is captured in the integrations test for sure, BUT I'll give this is real-life test for that confidence YES

edit: defining a support policy for GHES could be something worth considering

Yes, I remember that when you said it #910 (comment), But how do we go about it???

[babblebey]

So, I found a possible BREAKING CHANGE candidate for this PR, it's a fix for this #920

Here's what we're doing #920 (comment)

[travi]

But how do we go about it???

my thought is that this is basically just a documentation change and using the breaking release to make folks aware of the stance. since this would be specific to the github plugin, i think that documentation likely makes the most sense in the readme of this plugin. the fact that we bundle the github plugin into the core package by default potentially complicates some of that, though

we talked about adding checks against the api schema as part of our verification suite, which i think is valuable to follow through on, but i think that can be separate from defining the support range and documenting it.

however, i'm not sure we are well enough prepared to make such a stance as part of this release. the somewhat difficult decision that i think we need to consider is whether that is a rolling range based on GitHub's EOL schedule for GHES versions, or a static one where we would plan to make a breaking release each time we drop support for specific old versions. the latter option feels tedious without providing a lot of value, but the former option better communicates when an actual version falls out of our support in a way that better honors the intention of breaking.

[babblebey]

the fact that we bundle the github plugin into the core package by default potentially complicates some of that, though

Yea, following this BREAKING CHANGE, we still have to do a feature release to trigger this plugin update on core I believe

we talked about adding checks against the api schema as part of our verification suite, which i think is valuable to follow through on, but i think that can be separate from defining the support range and documenting it.

So, what do you say??? We go get this one merged... and Look towards planning the GHES Support policy, with a check in the verify lifecycle for it, and documenting supported range.

[babblebey]

and i see no reason to expect it not to work, but knowing that it is executed somewhere without resulting in an error would help some with confidence.

I thought to quickly let you know with the screenshot above... the logger.warn works... Note the DEPRECATION log parts

[travi]

So, what do you say??? We go get this one merged... and Look towards planning the GHES Support policy, with a check in the verify lifecycle for it, and documenting supported range.

i'm ok with this not making this major release. i think the other changes are only breaking for the github plugin, correct? i think it could be argued that the GHES support range definition would be as breaking for the core package since github is included by default, so in a way, including that with this release would make it more breaking than this release currently is

[gr2m]

🚀