Skip to content

Disable new merge freeze#5628

Merged
zwass merged 1 commit intomainfrom
zwass-remove-extra-freeze
May 6, 2022
Merged

Disable new merge freeze#5628
zwass merged 1 commit intomainfrom
zwass-remove-extra-freeze

Conversation

@zwass
Copy link
Copy Markdown
Member

@zwass zwass commented May 6, 2022

Keeping this off for now while we finish up the release with the existing tooling.

Keeping this off for now while we finish up the release with the existing tooling.
Copy link
Copy Markdown
Contributor

@fleet-release fleet-release left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repository is currently frozen for an upcoming release.

After the freeze has ended, please dismiss this review.

In case of emergency, you can dismiss this review and merge now.

@zwass zwass dismissed fleet-release’s stale review May 6, 2022 17:34

disabling this

@zwass zwass merged commit ed96be6 into main May 6, 2022
@zwass zwass deleted the zwass-remove-extra-freeze branch May 6, 2022 17:34
@mikermcneil
Copy link
Copy Markdown
Member

mikermcneil commented Jul 27, 2022

Connecting these PRs and issues together: This relates to #5179

Recent conversation on the topic:

mikermcneil: I thought we'd switched to using the GitHub bot I built instead of mergefreeze, which allowed for merging markdown PRs without waiting. Did something go wrong with that? Happy to help get it running again ASAP so we don't get a traffic jam again. cc @zach Wasserman

Zach Wasserman 🕘 4 hours ago The website bot didn't have the functionality we need (can't remember exactly what that was) and you were out so we switched back to mergefreeze.

Zach Wasserman 🕘 4 hours ago
What do we need right now that motivates switching back to the website bot?

Zach Wasserman 🕘 4 hours ago Frozen PRs can currently be unfrozen with the same process as before, which everyone has access to, on mergefreeze.com.

mikermcneil 4 hours ago We built the website bot because markdown-only PRs were backing up during the freeze period. It was designed to automatically check whether a PR is set up to be mergable during freeze based on files changed and then responded accordingly. This allowed us to quickly merge changes to the website, handbook, and docs during the freeze without logging into another system, and still allowing folks to dismiss reviews to be able to override the freeze while staying on GitHub. The bot also coached contributors on what to do to work around the freeze with language we could change.

Last I heard, that was working well, and without understanding what the problem was, it feels like a step backwards, since the bot gave us more control over how the freeze is applied with fewer steps and less training required for new contributors.

Do you remember where I could look to find out what the problem or limitation with the bot was? I'm happy to fix it.

Zach Wasserman 🕘 4 hours ago I can't remember exactly what the functionality was that existed in your tool, but I think this is basically what we need:

  • Anyone on the team can kick off the freeze.
  • All open PRs are frozen when the freeze is kicked off.
  • All new PRs are frozen during the freeze.
  • Frozen PRs show a failure status (or something else) that quickly prevents merging without manual intervention.
  • Anyone on the team can unfreeze a PR.
  • (Ideally, though mergefreeze.com doesn't support this) A reason is recorded for why a PR is being unfrozen.
  • All PRs are immediately unfrozen when the freeze ends (without requiring manual intervention from users).

mikermcneil 3 hours ago 👍 Awesome! All but the last two bullets are bullet is supported, and the last two one would be easy to add.

EDIT: second to last one is actually already supported since unfreezing a PR involves dismissing the review and providing a reason

Any member of our GitHub org (i.e. with permission to merge changes and request changes to the fleetdm/fleet repo) can enable/disable the freeze and run the freeze script with their GitHub token.

I can see that the fact that the last bullet not being yet implemented (a script to unfreeze all PRs rather than dismissing the review and merging) could have caused frustration. I wish I had been around to hear about that and fix it, sorry to leave you hanging.

Any other challenges solved in MergeFreeze but not the bot to be aware of that would prevent us from switching back?

Zach Wasserman 🕘 1 hour ago tbh I can't remember very well. I think it was mostly just mergefreeze fixed whatever the issue was we had with them and then switching back to it felt better at the time.

mikermcneil: Since I can't find the reasoning for going back to MergeFreeze, I'll assume there's something intangible about the tool that we like or a preference for using something different from the customizable GitHub responses (we can always switch to those later if we want to). So if mergefreeze's API allows for unfreezing particular PRs now (which I think they maybe added if memory serves?), then I'll just dig up the code from when I integrated with that the first time. Otherwise if that doesn't work well, then I'll finish the last bullet point above and hook it back up the way it was so that, either way, PRs to areas not intended to be frozen aren't affected by the freeze.
(I'll drop this in github for posterity too for future searching)

Zach Wasserman 🕘 15 minutes ago
If doing this work is lower effort than folks manually unfreezing PRs then it's fine with me.

Mike McNeil: 2022-07-26 @ 22:10 CT

If doing this work is lower effort than folks manually unfreezing PRs then it's fine with me.

Doing this work takes more effort than any one particular person unfreezing a particular PR. But when the repo is frozen with MergeFreeze, the count of unmerged, markdown-only PRs still starts climbing. This was why I set out to try and solve this a few months ago, especially when I heard the request that it would be helpful to reduce the total number of open markdown-only PRs, to make it less stressful in the repo.

One solution is train the Fleet team to use MergeFreeze, and ask them to pop into another tool to unfreeze particular PRs each time they merge. We've done that, but it hasn't worked at solving the problem yet. The company is 30+ people now, so that kind of thing can be hard to coordinate, and in the meantime, it slows down pull requests and it raises the risk of merge conflicts, especially for the people in the company who have the least amount of experience dealing with them. Even when trained, it can be discouraging to have to jump into another system, especially if your pattern of work is to make many small markdown pull requests throughout the day. It creates an incentive to wait until after the freeze to get work done.

This evening I integrated with the MergeFreeze API again, using their new support for freezing/unfreezing individual PRs. That, unfortunately, doesn't work as documented: #6905

I've emailed MergeFreeze and cced @zach Wasserman and @eashaw.

(I'll add the above in GitHub for future searching, and update there when there's progress to report so folks interested can stay up to date.)

For history of related changes and to reference old code, see:

  • Unfortunately, the MergeFreeze API doesn't work (EDIT: ...like I thought). Tool: Bring back mergefreeze API integration #6905 But it's still doable, just a little more awkward.
  • Fleet reverts back to using mergefreeze: ed96be6
  • Add script to satisfy next logical feature request: for freezing all PRs at same time: 5fe7fea
  • Build alternative to mergefreeze: adc7425
  • My initial pass at tjhe feature request from Engineering, to integrate with mergefreeze API to help reduce traffic jams and open numbers of PRs by allowing markdown PRs to be merged. Unfortunately, mergefreeze API didn't have what we needed at that time: adc52d3

mikermcneil added a commit that referenced this pull request Jul 27, 2022
Context: #5628 (comment)

Unfortunately, the API doesn't work.
mikermcneil added a commit that referenced this pull request Jul 27, 2022
* Tool: Bring back mergefreeze API integration

Context: #5628 (comment)

Unfortunately, the API doesn't work.

* Lay out how this would work in the database - but instead, do it ephemerally for now

* Remove model

* Maintain state (the easy way for now)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants