search

opencontainers / image-tools

OCI Image Tooling
https://opencontainers.org
Apache License 2.0
266 stars 83 forks source link

Revert "Relax LGTMs" #199

Closed stevvooe closed 4 years ago

stevvooe commented 6 years ago

This reverts commit e073febd8714374d6b459924cb0e2f2f998f6252.

This could only change with a proper vote by the project maintainers. Pushing this through with only 2 maintainers was not acceptable.

Furthermore, if there are problems getting PRs through, it usually has nothing to do with the number of maintainers on the project. It typically comes down to the level of engineering work put into making sure that a change is a clear and accurate solution to the problem. If it is unclear whether or not a change has impact, then the default reaction is to not merge the PR.

Signed-off-by: Stephen J Day stephen.day@docker.com

coolljt0725 commented 6 years ago

I'm agree with this revert. LGTM

FlorianLudwig commented 4 years ago

Furthermore, if there are problems getting PRs through, it usually has nothing to do with the number of maintainers on the project. It typically comes down to the level of engineering work put into making sure that a change is a clear and accurate solution to the problem. If it is unclear whether or not a change has impact, then the default reaction is to not merge the PR.

@stevvooe does that explain why this one never got merged?

cyphar commented 4 years ago

LGTM.

Approved with PullApprove

cyphar commented 4 years ago

@FlorianLudwig This project is no longer actively maintained, which is the core issue. Steven is right that in an actively-maintained project with multiple active maintainers, the 2-LGTM rule is rarely an issue. However, the project has the same maintenance issues as it did when the 1-LGTM change was merged, sort of proving Steven's point.