search

plantuml-stdlib / C4-PlantUML

C4-PlantUML combines the benefits of PlantUML and the C4 model for providing a simple way of describing and communicate software architectures
MIT License
6.38k stars 1.1k forks source link

Agree on review / merge guidelines #85

Closed Potherca closed 2 years ago

Potherca commented 3 years ago

As MRs are being opened and work commenses, I thought it might be good idea to address the topic of whom is "allowed" to merge. (As this is actionable, I created an Issue, rather than a Discussion)

I've seen this become a bottleneck in other projects caused by bystander apathy.

As we want to help @RicardoNiepel by taking over some of the load, my proposed guidelines are:

  1. Add the most active developers for a review (Currently @adrianvlupu, @aheil, @IOrlandoni, and @RicardoNiepel)
  2. When an MR has X approvals from those developers it can be merged.
  3. The author of the MR should merge their own MR (in case of conflicts they will be best suited to resolve things), unless someone else is waiting for the work from that MR.
  4. Don't worry about things too much. A merge to master is not the same as a release. There is always time to correct (or undo) anything that goes in the wrong direction. Better to keep moving forward than to have to wait for everyones approval.

I know all of this is implicitly more or less the way of doing things but I think it would be good to make it Explicit.

So what do you say? Agree/Disagree? Suggestions for improvement?

Tasks

adrianvlupu commented 3 years ago

Had no idea you can use checkboxes šŸ˜Š

I agree with all the above points.

I don't think @RicardoNiepel will have time to give approval but everyone else can join the process. I would be inclined to get the opinions of @aheil and @stawirej before a merge, seeing as they use it to communicate to lots of people.

RicardoNiepel commented 3 years ago

Perfectly fine for me - would recommend min 2 reviewers from the core team.

And make automatic tests required when they are merged/available.

aheil commented 3 years ago

Sounds absolute reasonable to me. Also I second "don't worry to much". šŸ˜ƒ

Potherca commented 3 years ago

@Crashedmind Can we get a Yay/Nay from you on this?

As we seem to be in agreement on this, I would suggest creating a CONTRIBUTING.md and writing them down there (either in this repo or in a organisation-wide .github repo šŸ¤”)

kirchsth commented 3 years ago

If you want, you could test your review rules, ... on my extensions. ;-) I'm basically finished with my extensions and would start with an update of the https://github.com/structurizr/dotnet-extensions/tree/master/Structurizr.PlantUML/IO/C4PlantUML as soon the macros(),... are fix.

kirchsth commented 3 years ago

Can we extend the review guidelines with a cyclic check of all pending PR's (e.g. 2 weeks). If you want, you could test the extended review rules, ... on my PR's ;-)

Potherca commented 3 years ago

Regarding 2 reviewers from the core team.... This is becoming a bottleneck for changes being merged.

Some PR are open so long that the stale-bot is triggered!

I think we either need to find a way for contributors to actually commit to reviewing, add a time-limit for how long an MR has to wait for a second reviewer, or reduce the limit to 1.

kirchsth commented 3 years ago

@Potherca: Can we create a new release (independent of other reviewers) that the new features can be merged into PlantUML and it can be used with kroki too (#156)?