Hi, guys. I believe that we should update maintainers list before adding this review requirement. There should not be the situation when there is only one contributor who is responsible for a component. We already have issues with review speed and response time.
On Fri, Apr 20, 2018 at 2:17 PM, Anton Vinogradov <a...@apache.org> wrote: > Vova, > > Everything you described sound good to me. > > I'd like to propose to create special page at AI Wiki and to describe > checklist. > In case we'll find something should be changed/improved it will be easy to > update the page. > > 2018-04-20 0:53 GMT+03:00 Nikolay Izhikov <nizhi...@apache.org>: > > > Hello, Vladimir. > > > > Thank you for seting up this discussion. > > > > As we discussed, I think an important part of this check list is > > compatibility rules. > > > > * What should be backward compatible? > > * How should we maintain it? > > > > > 3) If ticket changes public API or existing behavior, at least two > > commiters should approve the changes > > > > We can learn from other open source project experience. > > Apache Kafka [1], for example, requires KIP(kafka improvement proposal) > > for *every* major change. > > Major change definition includes public API. > > > > [1] https://cwiki.apache.org/confluence/display/KAFKA/ > > Kafka+Improvement+Proposals > > > > > > В Чт, 19/04/2018 в 23:00 +0300, Vladimir Ozerov пишет: > > > Hi Igniters, > > > > > > It's glad to see our community becomes larger every day. But as it > grows > > it > > > becomes more and more difficult to manage review and merge processes > and > > > keep quality of our decisions at the proper level. More contributors, > > more > > > commits, more components interlinked with each other in subtle ways. > > > > > > I would like to propose to setup a formal review checklist. This would > > be a > > > set of actions every reviewer needs to check before approving merge of > a > > > certain feature. Passing the checklist would be *necessary but not > > > sufficient* phase before commit could be added to the main branch. The > > > checklist would help us to detect a lot of common problems such a > broken > > > tests or bad UX earlier, and would help contributors lead their pull > > > requests to merge. > > > > > > Hallmarks of a good checklist: > > > - It must be followed be everyone without exceptions > > > - It must be clear and disallow multiple interpretations > > > - It must be lightweight, otherwise Ignite development would become a > > > nightmare > > > - It must be non-blocking, i.e. inacessibility of a single contributor > > > should not block ticket progress forever. > > > > > > Please let me know if you think the idea makes sense. If we agree on > it, > > > let's start defining action items for the checklist. My 2 cents: > > > 1) All unit tests pass on TC without new failures > > > 2) If ticket targets specific component, it should be reviewed by > > > component's maintainer* > > > 3) If ticket changes public API or existing behavior, at least two > > > commiters should approve the changes ** > > > > > > Thoughts? > > > > > > Vladimir. > > > > > > * TBD: Review component list and define maintainers; define what to do > if > > > maintainer is unavailable > > > ** TBD: Define what is "public API" > > >
