Skip to content
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

Improve and automate cherry pick process #53

Closed
elicwhite opened this issue Nov 13, 2018 · 6 comments
Closed

Improve and automate cherry pick process #53

elicwhite opened this issue Nov 13, 2018 · 6 comments
Labels
🗣 Discussion This label identifies an ongoing discussion on a subject

Comments

@elicwhite
Copy link

I'd like to start a conversation around how we can make it easier to do patch releases. My understanding is that a lot of this work comes from trying to manage cherry pick requests, ensure things merge cleanly, and work end-to-end.

Right now this seems like a very manual process. People comment with a commit they want merged, and then the people running the releases (for at least the last few patch versions this has been @kelset) try to cherry pick this commit onto the release branch. Often this doesn't merge cleanly and the maintainer is responsible for finding the other commits that are required to make it merge cleanly. Once it is on the branch they go through a manual process to validate that "everything" works. Essentially manually running our end to end flows.

A lot of other projects that do cherry picks have some automation set up to make this easier. I think we should too.

A bot could take the cherry pick requests and open PRs against the release branch. PRs kick off all the normal tests which means that as we beef up our tests this stage will be validated better as well. Community members are able to merge PRs against release branches so unlike normal PRs, these can be handled without someone from Facebook reviewing each one.

Would someone be interested in taking point on trying to identify the ideal workflow and if there are preexisting bots that we can use for this or if we need to build one ourselves?

@kelset kelset added the 🗣 Discussion This label identifies an ongoing discussion on a subject label Nov 14, 2018
@kelset
Copy link
Member

kelset commented Nov 14, 2018

Thanks Eli for starting this conversation!

I think that, yes, your description of the current flow is correct and yes there is a lot of manual work involved: but this mostly spikes when a commit request hits a merge conflict or creates a failure. So - ideally - the bot should:

  • detect the cherry-pick request
  • test the cherry pick (via a PR? is there a way to test that woulnd't involve a PR? asking simply to avoid too much noise in the PR section of the main repo)
  • if the CI goes red, or there are merge conflicts, the bot mentions the author of the request and asks him to create his own PR to fix the issues
  • request author tags bot, bot merges "clean" PR

Aside from the problem of a bot being able to do all of these things, my main concern is the 'when' of it all: probably it should not happen when the comment gets created. It should instead happen when a release decides to be cut: this because I feel that to avoid issues, the bot should parse all the cherry pick requests, sort the commits chronologically and then try to merge them in the stable branch.

(related conversations: react-native-community/releases#47 && #17)

@grabbou
Copy link
Member

grabbou commented Nov 17, 2018

Happy to see this conversation starting!

From my point of view (as well as some of the historical React Native releases), I always appreciated merge conflicts and started treating them as a sign that something non-trivial is up in the air. Most of the time, a merge conflict would appear when there was a commit to be applied to the release branch that was based on another commit that wasn't there yet. An example would be asking to cherry-pick a commit to fix a FlatList issue that has been introduced by a commit that wasn't included in the release branch yet.

In the ideal scenario, for each series of commits, a PR would be open. The bot would automatically CC the maintainer that fired the command and the person that requested the cherry-pick to look at possible failures before it ships.

My concern with this process is the time and possible delays that could be caused. This is only related to my vision of this process and understanding I have from your description.

First of all, not all the people that request cherry-picks are familiar with the React Native codebase enough to do so. They simply see a commit that fixes the issue they are facing and ask for it to be ported. That means either a maintainer would have to resolve conflicts (this is what we do now) or an original author that committed the code (this is what we do sometimes when former doesn't work due to lots of moving pieces).

Another case would be when a person wouldn't be responsive enough (from the time perspective) to resolve conflicts on the PR. Usually, when we cherry-pick commits, we do it all at once during a short time frame. Relying on a PR-system would mean we skip important commits from being cherry-picked for a while.

And the last concern - there are cases when commits to be cherry-picked are linked together or dependent on each other. Assuming we open many PRs in parallel for every commit, merging one of them could potentially cause another conflict to be resolved again on the other one. Can become an issue when two aforementioned come into play.

Regarding @kelset concern:

Aside from the problem of a bot being able to do all of these things, my main concern is the 'when' of it all

I think a simple facebook-github-bot pick sha1 would do!

@elicwhite
Copy link
Author

Thanks for the perspective @grabbou. It sounds like you don’t believe the approach in the initial comment would work.

Do you have a recommendation for how the current workflow could be improved with tooling to reduce the work that you and @kelset have been doing and for it to be easier for other people to get involved and help out?

How do other major projects handle releases and cherry picks? I imagine they have very similar problems and have tooling to make it easier.

@grabbou
Copy link
Member

grabbou commented Nov 17, 2018

Hey Eli! Thanks for answering!

It's not that I don't believe in it - I just wanted to put a few questions to be answered to help us get this forward! I really like this idea and I think there's work that can be done around this process to make our lives easier.

For example, one of the concerns I have raised regarding PRs coupled together could be solved by allowing the bot to accept related commits to be included in the same PR. We could take this step even further by enhancing the bot algorithms with simple heuristics so that it can detect related commits itself and group them in a single PR automatically. That could potentially solve the other problem, which is, detecting merge conflicts and understanding why they did happen. This could be either Github bot or a CLI command we run (in a similar to ./bump-oss-version.js fashion).

Now, I didn't leave any alternative proposals in my comment as I don't really have any at this point. I am going to do my homework in the upcoming days and see what are the processes that other projects settled on and whether there's any tooling that they use to make it even better.

I think we are moving in the right direction - now, it's time to get to the details!

@grabbou
Copy link
Member

grabbou commented Jan 28, 2019

Another important thing to remember (which I often caught myself) - cherry picking a commit that has been rolled back. Case was cutting 0.58 that shipped with a bug that has been rolled back few days after. I am not sure what could be a solution to this problem, but we could have a better communication with Facebook internally when they close their review window.

@cpojer
Copy link
Member

cpojer commented Apr 29, 2019

I'm currently writing up a document for RN's GitHub repo that explains the release process as discussed in #17. I don't think for now we'll build automated tooling to handle cherry picks and I think we outlined a few things in that issue that will make releases a bit easier to deal with so I'll close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗣 Discussion This label identifies an ongoing discussion on a subject
Projects
None yet
Development

No branches or pull requests

4 participants