chore(deps): bump minimum peer dependency bound with semantic-release
#916
Conversation
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. |
Yea, this I did with 85d0ad0 and 0843ffe but the PR was failing CI with the error below in some unit tests.... ...hence I thought to revert.
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? |
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
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.
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. |
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:
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 |
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.
I am very interested in this for sure 😃
Yea, I'll do some digs |
...and It didn't take any longer than 40secs to figure that I was missing the mock
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
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. |
|
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 |
Yea, will do... gotta be the case since we're merging commits 😉
Yea, it is captured in the integrations test for sure, BUT I'll give this is real-life test for that confidence YES
Yes, I remember that when you said it #910 (comment), But how do we go about it??? |
BREAKING CHANGE: the minimum required version of semantic-release to use @semantic-release/github is now v24.1.0
|
So, I found a possible Here's what we're doing #920 (comment) |
…rty data type. BREAKING CHANGE: the commit associatedPR and relatedIssues `label` prop is now an array of objects with more properties
…ommentCondition`; updated information on relatedIssue type of issue
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. |
Yea, following this BREAKING CHANGE, we still have to do a feature release to trigger this plugin update on core I believe
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 thought to quickly let you know with the screenshot above... the |
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 |
|
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR Bumps the minimum bound of the
semantic-releasepeer dependency in order to allow the plugin to consume the newwarnfunction added in semantic-release/semantic-release#3423BREAKING CHANGE: the minimum required version of
semantic-releaseto use@semantic-release/githubis now v24.1.0Related Issue
Resolves #902
Fixes #920