Re: [openstack-dev] [Fuel] Code merge policy
I don't think requiring a +1 from every single reviewer listed on the review before merging it is reasonable. Every day a review that is ready to be merged spends waiting means more time spent rebasing, redoing the reviews, dealing with immediate and hidden problems caused by changes in master branch. It also linearly increases the number of reviews we have pending at a time, and combinatorially increases the number of dependencies and conflicts between them all. None of that comes for free. We should be looking for ways to improve quality of our reviews without increasing the time commits spend waiting for review. In case of this particular commit, the solution could have been much simpler: have a meaningful commit message that enumerates all goals, significant changes, and external dependencies of the commit. I also agree with Mike on a need for a full CI gate job for all of our repositories, not just fuel-library. Things like serializer changes (e.g. https://review.openstack.org/83204) can happily pass unit tests and still cause deployment failures. No matter how much more code review we do, things like that will slip through, only CI can ensure that a reasonable code change in nailgun, astute, or ostf won't cause a deployment failure by breaking an expectation in fuel-library. On Tue, Apr 15, 2014 at 9:02 AM, Dmitry Pyzhov dpyz...@mirantis.com wrote: So every developer should manually mark every such review as WIP? And remove this flag only when everyone agreed to merge? This will require additional actions in 50% of fuel-web and 100% of fuel-main reviews. Developers make mistakes too. Let's just be more accurate. On Tue, Apr 15, 2014 at 6:41 PM, Mike Scherbakov mscherba...@mirantis.com wrote: Humans make mistakes... all the time. Let's think how we can automate this to have appropriate Jenkins check. In this particular case, we could do the following: a) make it work in progress if we still unsure on some deps b) can we have smoke test which would check that master node builds, and simplest deploy passes? This needs to be run only if there are changes discovered in ISO build script (including mirror changes), and puppet manifests which deploy master node On Tue, Apr 15, 2014 at 3:49 PM, Dmitry Pyzhov dpyz...@mirantis.com wrote: Guys, We have big and complicated structure of the project. And part of our patchsets require additional actions before merge. Sometimes we need approve from testers, sometimes we need merge requests in several repos at the same time, sometimes we need updates of rpm repositories before merge. We have informal rule: invite all the required persons to the review. And core reviewer does not merge code if part of +1's are missed. Sad, but this rule is not obvious. This informal rule became even more strict when we need update of rpm/deb repositories, because OSCI changes should be accomplished right before merge. For such reviews we ask OSCI team to do changes, checks and merge. https://review.openstack.org/#/c/86001/ This particular request requires check of our 4.1.1 rpm/deb repositories status. Thats why Roman Vyalov is added as reviewer. I don't like over-bureaucracy. My suggestion is simple: take into account reviewers status and do not merge if unsure. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Mike Scherbakov #mihgen ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Dmitry Borodaenko ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Fuel] Code merge policy
Humans make mistakes... all the time. Let's think how we can automate this to have appropriate Jenkins check. In this particular case, we could do the following: a) make it work in progress if we still unsure on some deps b) can we have smoke test which would check that master node builds, and simplest deploy passes? This needs to be run only if there are changes discovered in ISO build script (including mirror changes), and puppet manifests which deploy master node On Tue, Apr 15, 2014 at 3:49 PM, Dmitry Pyzhov dpyz...@mirantis.com wrote: Guys, We have big and complicated structure of the project. And part of our patchsets require additional actions before merge. Sometimes we need approve from testers, sometimes we need merge requests in several repos at the same time, sometimes we need updates of rpm repositories before merge. We have informal rule: invite all the required persons to the review. And core reviewer does not merge code if part of +1's are missed. Sad, but this rule is not obvious. This informal rule became even more strict when we need update of rpm/deb repositories, because OSCI changes should be accomplished right before merge. For such reviews we ask OSCI team to do changes, checks and merge. https://review.openstack.org/#/c/86001/ This particular request requires check of our 4.1.1 rpm/deb repositories status. Thats why Roman Vyalov is added as reviewer. I don't like over-bureaucracy. My suggestion is simple: take into account reviewers status and do not merge if unsure. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Mike Scherbakov #mihgen ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Fuel] Code merge policy
So every developer should manually mark every such review as WIP? And remove this flag only when everyone agreed to merge? This will require additional actions in 50% of fuel-web and 100% of fuel-main reviews. Developers make mistakes too. Let's just be more accurate. On Tue, Apr 15, 2014 at 6:41 PM, Mike Scherbakov mscherba...@mirantis.comwrote: Humans make mistakes... all the time. Let's think how we can automate this to have appropriate Jenkins check. In this particular case, we could do the following: a) make it work in progress if we still unsure on some deps b) can we have smoke test which would check that master node builds, and simplest deploy passes? This needs to be run only if there are changes discovered in ISO build script (including mirror changes), and puppet manifests which deploy master node On Tue, Apr 15, 2014 at 3:49 PM, Dmitry Pyzhov dpyz...@mirantis.comwrote: Guys, We have big and complicated structure of the project. And part of our patchsets require additional actions before merge. Sometimes we need approve from testers, sometimes we need merge requests in several repos at the same time, sometimes we need updates of rpm repositories before merge. We have informal rule: invite all the required persons to the review. And core reviewer does not merge code if part of +1's are missed. Sad, but this rule is not obvious. This informal rule became even more strict when we need update of rpm/deb repositories, because OSCI changes should be accomplished right before merge. For such reviews we ask OSCI team to do changes, checks and merge. https://review.openstack.org/#/c/86001/ This particular request requires check of our 4.1.1 rpm/deb repositories status. Thats why Roman Vyalov is added as reviewer. I don't like over-bureaucracy. My suggestion is simple: take into account reviewers status and do not merge if unsure. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Mike Scherbakov #mihgen ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
