Moya: Accidental push to master

[justinmakaila]

@Moya/owners

Can someone help me undo the latest commit to master? I can't force push to a protected branch, wondering if there was some other way than manually undo-ing it, which I'm open to because I messed it up.

🤦‍♂️

[justinmakaila]

@Moya/contributors as well

[justinmakaila]

I'm trying to use git revert, but I'm not entirely familiar. The ... reset ... push -f didn't work

[justinmakaila]

I ran git revert master -m 1, but haven't pushed it yet because i don't want to undo the wrong stuff.

[ashfurrow]

A revert generates a new commit, to be safe it'd be best to put that commit in a PR, if that works for you? I'm happy to do it but I'm away from a computer right now.

[scottrhoyt]

Hey @justinmakaila, are you familiar with how to generate a reverse commit? As @ashfurrow mentioned, you should be able to push that to master without force pushing anything. If you need any help or would like me to do it, post the hash of the commit you want reversed and I can do it. Thanks!

[BasThomas]

I would think this concerns ca5c9fed3a6ad4bb8be9a5d882bb202782910ac1, right @justinmakaila?

[justinmakaila]

Exactly, @BasThomas.

@scottrhoyt, @ashfurrow, I'm fine with PR'ing it, I just wanna make sure the command I'm running is legit.

Quick patch: Is there any reason we allow direct pushes to master? I know, realistically, I should have watched what I was pushing to, but is there anyway we can protect against this in the future?

[ashfurrow]

Yup, cool so on a branch you do git revert ca5c9fed3a6ad4bb8be9a5d882bb202782910ac1, save the default commit message, and then push your branch to do a PR as usual.

We allow pushes to master because the release script, which modifies the changelog and podspec, commits and pushes. With this being automated, maybe there's an opportunity to change that.

Let me know if I can help or further clarify anything 👍

[justinmakaila]

@ashfurrow Well here's the fun part... ca5c9fe is actually a merge commit... I had my own fork of Moya with tweaks for whatever version of ReactiveSwit I was using. I pulled in upstream (this repo), and merged, then, thinking I was pushing to my branch, accidentally typed git push upstream master which pushed whatever just got merged in with whatever out of date changes I had.

The parents of the commit are 4160bb6 + b0a70e2, where b0a70e2 is the commit that I want to revert master back to (i.e. undo whatever was merged in from 4160bb6.

I believe the proper revert command would be git revert -m 1 ca5c9fe, which gives me the message:

Revert "Merge in master"

This reverts commit ca5c9fe..., reversing changes made to 4160bb6...

I'm a little tentative to pull the trigger right now, seeing as I've already ruined our history a bit ;).

I'm going to create a new fork (I don't trust the history of my old one anymore...), create a new branch, run the revert command above, and then file a PR.

[scottrhoyt]

Try diffing the branch with the reverted commit against the last commit before your accidental one.

[ashfurrow]

Yikes, I've never successfully reverted a merge commit except through the GitHub UI. Hmm. Any advice here would be welcome.

[justinmakaila]

@Moya/contributors after further review, it looks like my branch was used as the base, and the "merge" was really just a fast forward, that looks weird because my branch was used last the base.

I changed one of my projects to point to master, and have been using it with seemingly no issue.

[justinmakaila]

Ultimately, I think this demonstrates that we need to lock down the master branch and ensure that the only code that can get into master is via PR and approval from the organization.

[AndrewSB]

I agree with Justin about locking down master, especially since the Moya
organization is huge, which introduces more potential points of failure. Is
the release script the only consumer committing directly to master? Can we
have it make a Pull Request instead?

On Fri, Mar 3, 2017 at 3:03 AM Justin Makaila [e-mail]
wrote:

Ultimately, I think this demonstrates that we need to lock down the master
branch and ensure that the only code that can get into master is via PR and
approval from the organization.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Moya/Moya/issues/994#issuecomment-283788926, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADo1dKXew-BYKg2aq7F5ck9pDwXDw5_8ks5rhzW0gaJpZM4MPIc3
.

[ashfurrow]

GitHub's control over branch protection looks fairly granular – we we automate the release, get @ashfurrowbot or @moyabot or someone's bot to push, and whitelist them as having direct push access. We've been discussing automating our release process a bit in https://github.com/Moya/Moya/issues/983 but could someone open a new issue for just it?

[BasThomas]

Yeah, if we can do releases via @moyabot, that'd be awesome!

[ashfurrow]

There may be a complication using @moyabot actually. It's purposefully not a member of the org because it has its personal access token to access repos public (it _has no repos_ to access, so it's fine). It needs this for Danger integration. Hmm. I can't really use @ashfurrowbot either for the same reason. Not sure about next steps.

[scottrhoyt]

Well, I suppose on the other hand, as long as we can't force push, then we won't be able to do anything absolutely irreversible. It just might be a major pain to do so...

[orta]

A deploy isn't contingent on pushing to master - just the tag + commits, so
amending the script to put you in a place to send a private seems a good
first step?

On Mar 4, 2017 12:49 PM, "Scott Hoyt" [e-mail] wrote:

Well, I suppose on the other hand, as long as we can't force push, then we
won't be able to do anything absolutely irreversible. It just might be a
major pain to do so...

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Moya/Moya/issues/994#issuecomment-284124706, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAC_jswdyUaejOlcBq5KHtllS9FD6AfVks5riN9AgaJpZM4MPIc3
.

[ashfurrow]

Yeah, we'd have to modify our deploy script because it currently updates the changelog and podspec, commits those changes, and pushes. If we did something else like using PRs for those changes, and then tag post-merge, then CI can just push to CocoaPods and make a GitHub release. Sounds like a plan to me! Alternatively, we could use deploy keys instead of using personal access tokens; it's more involved but we can automate away the PR process.

@scottrhoyt would you be interested in making those changes? Happy to help but I'm also keen to see the project grow on its own, without me :smile:

[scottrhoyt]

Sure, @ashfurrow. My other workload has picked up a bit, so it might be a few days before I can look at this, but happy to give it a go.

[ashfurrow]

Cool cool, much appreciated. No pressure remember!

[BasThomas]

Does anyone have an idea on how to revert the accidental push? I think that would be good to figure out as well.

[justinmakaila]

@BasThomas The issue (I believe) was that I was running my own fork, and my own version of master, so when I went to merge in, it used my branch as the base, and merged in this repo's master, which pretty much just merged my four month old commits into the history.

I attempted to use the standard suggested git revert and whatnot, but since my branch was the base for the merge, it was threatening to undo months of work on this branch.

Given that context, if anyone who understands git a little better has an idea, it should be pretty easy to drop those commits from the history (I was literally just tweaking the Cartfile to match ReactiveSwift with the version of ReactiveCocoa I was using), then that would provide a more accurate representation of development over time, but I'm not sure if it's necessary, given that all of the files I edited were definitely manipulated and changed since I last touched them.

[BasThomas]

Yeah, I didn't notice any file changes in the project itself, so it should be fine.

[sunshinejr]

I think this should be resolved, so I'm gonna close it for now.