8000 fix: address `@octokit/rest` deprecations by gr2m · Pull Request #249 · semantic-release/github · GitHub
[go: up one dir, main page]

Skip to content

fix: address @octokit/rest deprecations #249

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

Merged
merged 4 commits into from
Feb 3, 2020

Conversation

gr2m
Copy link
Member
@gr2m gr2m commented Feb 3, 2020

closes #248

@gr2m
Copy link
Member Author
gr2m commented Feb 3, 2020

I'm confused I don't understand why the tests are failing, I'll have to digg some more into it

@gr2m gr2m force-pushed the 248/address-octokit-rest-deprecations branch 2 times, most recently from cb6c532 to 7e61d9a Compare February 3, 2020 01:57
@gr2m
Copy link
Member Author
gr2m commented Feb 3, 2020

Something very funky is going on. I'll continue looking into it tomorrow.

It seems that the tests pass again without my other changes: #248 (comment) so there is no urgency

@gr2m gr2m force-pushed the 248/address-octokit-rest-deprecations branch from 7e61d9a to 58184ac Compare February 3, 2020 04:58
@gr2m
Copy link
Member Author
gr2m commented Feb 3, 2020

All right, this one is ready

@gr2m gr2m requested a review from pvdlg February 3, 2020 05:06
@gr2m gr2m force-pushed the 248/address-octokit-rest-deprecations branch from 58184ac to 72970f4 Compare February 3, 2020 06:24
@pvdlg
Copy link
Member
pvdlg commented Feb 3, 2020

Can you squash your commits before merging (as in don't split changes between fix and test if the tests changes are related to the fix)?

The goal is to have atomic commits, so in the future when we look at the git history we know what goes with what. That also prevent mistakes in case we have to rollback a change. With atomic commits we can rollback any one commit without have to worry about forgetting to also rollback another one.

@gr2m
Copy link
Member Author
gr2m commented Feb 3, 2020

I usually split them out so I have a test commit that has failing CI and then a fix/feature commit that is green, to make sure that the test is actually doing what it is supposed to do. In this case I had to push --force to clean up commits

If you like we can enforce squash merging for the semantic-release repos.

Other than that, is this good to merge?

@pvdlg
Copy link
Member
pvdlg commented Feb 3, 2020

During the PR phase we can do pretty much whatever we want in terms off commits. What's important is for the commits that ends up on master to be atomic.

We could enforce squash merging , but there might some rare cases were we open one PR and do two things. For example I happened to have PR that both increase the minimum node version and update something else. In that case I keep 2 commits, 1 for the Node upgrade, 1 for the other thing.

Regarding the code changes, yes we can merge. Not sure if #250 is necessary though, we can upgrade to v17 once it's out of beta.

@gr2m
Copy link
Member Author
gr2m commented Feb 3, 2020

We could enforce squash merging , but there might some rare cases were we open one PR and do two things. For example I happened to have PR that both increase the minimum node version and update something else

In these cases I often times edit the release notes after the release

Not sure if #250 is necessary though, we can upgrade to v17 once it's out of beta.

It was just me testing, I'm not planning on merging it until v17 stable is released

@gr2m gr2m changed the title address @octokit/rest deprecations fix: address @octokit/rest deprecations Feb 3, 2020
@gr2m gr2m merged commit c1ae4ac into master Feb 3, 2020
@gr2m gr2m deleted the 248/address-octokit-rest-deprecations branch February 3, 2020 17:13
@semantic-release-bot
Copy link
Collaborator
semantic-release-bot commented Feb 3, 2020

🎉 This PR is included in version 7.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

An in-range update of @octokit/rest is breaking the build 🚨
3 participants
0