spiral-project / ihatemoney

A simple shared budget manager web application
https://ihatemoney.org
Other
1.18k stars 267 forks source link

Release management and pull request merging rules #1245

Closed almet closed 10 months ago

almet commented 12 months ago

Hi, in #1243 I merged a pull request too early, and it seems it could help to decide on how we should merge to master, release and issue bugfix releases.

I believe these scenarii will probably happen more often than before, as more people are working on the repository than usual. We might want to setup some rules for merging, releasing, etc.

  1. Maybe the simpler option of all : wait before either 48h has passed or two people have reviewed a pull request before merging. We could use the "approve" mechanism of github here;
  2. In other projects, I've used the main branch for development, and sometimes issue bugfix releases on specific branches, where we cherry-pick commits of interest ;
  3. Any other option I haven't thought of, I'm all ears :-)

Again, sorry if I caused some confusion here, let's discuss the issue and find an solution :-)

azmeuk commented 12 months ago

Maybe the simpler option of all : wait before either 48h has passed or two people have reviewed a pull request before merging. We could use the "approve" mechanism of github here;

The "wait for X time, or have Y maintainers approval" seems indeed like a good and simple rule. 2 maintainers seems fine, but 2 days feels be a bit short to me (in case people want to unplug their computer, take vacations etc.). What about 1 week?

almet commented 12 months ago

2 maintainers seems fine, but 2 days feels be a bit short to me (in case people want to unplug their computer, take vacations etc.). What about 1 week?

Seems fine to me :-)

zorun commented 12 months ago

Sorry if my initial reaction was a bit harsh, I'm happy you opened this to discuss it.

Yes, I'm all for this rule "2 maintainers approval required, but after one week just 1 maintainer approval is enough".

I don't think we need to enforce it technically, but we should document it.

About the dev/release strategy, we had a stable branch once (stable-4.1), but that was kind of a special situation: we had discovered a very serious security issue while the master branch was quite unstable and not fit for a release, so we created this stable branch based on the previous release. If we want to generalize this, it's additional work.

almet commented 12 months ago

Thanks for your feedback. No harm taken here. I perfectly understand your reaction, don't worry.

2 maintainers approval required, but after one week just 1 maintainer approval is enough.

That seems an agreement. @Glandos, do you have any take on this?

As for the dev/release strategy, I believe delaying things will make it easier to deal with bugfix releases, so let's not make things more complex that needed for now, I would say.

almet commented 11 months ago

Okay, I'm going to close this and consider this the way we work now. @Glandos don't hesitate to reopen if you feel the need.

zorun commented 11 months ago

@almet can you write down these rules in the "contributing" section of our docs?

almet commented 11 months ago

You're right, I forgot we should document it in the documentation =D