Re: [xwiki-devs] [Proposal] GitHub code reviews and protected branches
I really don't believe in mandatory pull requests for everyone, it always sounds good in theory but in practice it would just be a frustrating nightmare for core committers (at best we'll just validate each other pull request without really looking at them much most of the time defeating the whole purpose). I prefer trusting core committers to create pull requests (or ask for branch review) when they are not confident enough with what they did (which they currently do). On the lets make master branch great again, it would sure be nice to have build results for branches/pullrequests on commons/rendering/platform/enterprise (I'm very happy to have this on Contrib extensions). But with the current flickerings tests we have in some UI tests, an automated merge is simply not applicable right now. It can only be an information (a very useful one) given to whoever created the branch before merging it. But as Edy pointed out it should not be over used as Jenkins already suffer quite a bit right now (unless we boost ci infrastructure of course). Also keep in mind that in a multi repositories projects like XWiki you can't fully rely on build result of your commit/branch's repository, xwiki-platform build is often broken by a changes made to xwiki-commons and xwiki-enterprise build failures almost never have anything to do with one of its commits (even if xwiki-enterprise repository should vanish is not too long but it won't improve the already very long platform build). On Wed, May 24, 2017 at 1:08 AM, Eduard Moraruwrote: > Hi, > > Very interesting discussion. > > Not much to add to what was already said. > > I have always questioned the fact that we keep the master branch as the > main development branch and it is very easy for someone new to clone it at > the wrong time (i.e. when the build is failing) and get confused fast. Most > github projects I see use the master branch as a stable-development branch > to which they merge reviewed code or code from other active-development > branch(es). > > Side note: One really nice feature that I would like to see on GitHub would > be the ability to review already pushed commits (i.e. a "post-mortem" > review). You would then have a nice overview of code that was *never* > reviewed by anyone and could easily develop a workflow that tolerates > direct pushes, but requires that even they are eventually reviewed > (possibly with some Review Percentage Coverage minimum limit). I`m not sure > I`m such a big fan of PR-only commits. Ultimately, I think I would even be > in favor of getting your code in faster and fixing/reverting in case of > problems (i.e. the opposite direction) instead of raising even more > barriers in front of the contribution/commit. > > General question about building-checking each PR: If PR1 is built by CI > automatically and is marked as PASSED, but commit A is pushed into master > and it would make PR1 now fail the build, is PR1 re-built after commit A is > pushed to master? If so, does it mean that each commit to master also > triggers a rebuild of all the N PRs waiting for review to make sure they > are not broken? AFAIU, this is the way things should work, however, given > the elegant behemoth that XWiki has become, the build times and the stress > on the CI infrastructure would IMO kill any fun and joy of developing and > actually making a change in the project. > > I`m all for Code Reviews and a system that improves the current state where > nobody is really responsible with it and, if it happens, it is generally > done by the same few people. > > The way I imagine the perfect review system is one where each commit and > each PR gets a reviewer automatically assigned from the organization (other > than the author, ofc). PRs are blocked by the reviewer's approval and > direct commits should (maybe/somehow?) be blocked by a Review Coverage > Threshold, similar to the TPC at build time. I believe that if we could > achieve this, it would invalidate the need for more complex branch/fork > management and other formalities. With a bit of an effort and maybe GitHub > API magic, I believe it could even be achievable to a certain extent. > > As you`ve said, the ultimate goal is for people to see the code and avoid > finding things introduced 7 years ago that nobody *ever* had a second look > at. > > Hope this helps, somehow :) > > -Eduard > > On Tue, May 23, 2017 at 9:05 AM, Sergiu Dumitriu wrote: > >> Hi Vincent, >> >> On 05/17/2017 09:30 AM, Vincent Massol wrote: >> > Hi Sergiu, >> > >> >> On 16 May 2017, at 07:36, Sergiu Dumitriu wrote: >> >> >> >> Hi devs, >> >> >> >> A while ago GitHub introduced several features that I think can help >> >> boost even more the code quality, reduce the bus factor, and make it >> >> more attractive to contribute to the project. >> >> >> >> = Protected branches = >> >> >> >> Branches can be configured as protected in several ways: >> >> * Require pull
Re: [xwiki-devs] [Proposal] GitHub code reviews and protected branches
Hi, Very interesting discussion. Not much to add to what was already said. I have always questioned the fact that we keep the master branch as the main development branch and it is very easy for someone new to clone it at the wrong time (i.e. when the build is failing) and get confused fast. Most github projects I see use the master branch as a stable-development branch to which they merge reviewed code or code from other active-development branch(es). Side note: One really nice feature that I would like to see on GitHub would be the ability to review already pushed commits (i.e. a "post-mortem" review). You would then have a nice overview of code that was *never* reviewed by anyone and could easily develop a workflow that tolerates direct pushes, but requires that even they are eventually reviewed (possibly with some Review Percentage Coverage minimum limit). I`m not sure I`m such a big fan of PR-only commits. Ultimately, I think I would even be in favor of getting your code in faster and fixing/reverting in case of problems (i.e. the opposite direction) instead of raising even more barriers in front of the contribution/commit. General question about building-checking each PR: If PR1 is built by CI automatically and is marked as PASSED, but commit A is pushed into master and it would make PR1 now fail the build, is PR1 re-built after commit A is pushed to master? If so, does it mean that each commit to master also triggers a rebuild of all the N PRs waiting for review to make sure they are not broken? AFAIU, this is the way things should work, however, given the elegant behemoth that XWiki has become, the build times and the stress on the CI infrastructure would IMO kill any fun and joy of developing and actually making a change in the project. I`m all for Code Reviews and a system that improves the current state where nobody is really responsible with it and, if it happens, it is generally done by the same few people. The way I imagine the perfect review system is one where each commit and each PR gets a reviewer automatically assigned from the organization (other than the author, ofc). PRs are blocked by the reviewer's approval and direct commits should (maybe/somehow?) be blocked by a Review Coverage Threshold, similar to the TPC at build time. I believe that if we could achieve this, it would invalidate the need for more complex branch/fork management and other formalities. With a bit of an effort and maybe GitHub API magic, I believe it could even be achievable to a certain extent. As you`ve said, the ultimate goal is for people to see the code and avoid finding things introduced 7 years ago that nobody *ever* had a second look at. Hope this helps, somehow :) -Eduard On Tue, May 23, 2017 at 9:05 AM, Sergiu Dumitriuwrote: > Hi Vincent, > > On 05/17/2017 09:30 AM, Vincent Massol wrote: > > Hi Sergiu, > > > >> On 16 May 2017, at 07:36, Sergiu Dumitriu wrote: > >> > >> Hi devs, > >> > >> A while ago GitHub introduced several features that I think can help > >> boost even more the code quality, reduce the bus factor, and make it > >> more attractive to contribute to the project. > >> > >> = Protected branches = > >> > >> Branches can be configured as protected in several ways: > >> * Require pull request reviews before merging > >> ** no direct commits are allowed, everything must go through a pull > request > >> ** a pull request must be reviewed by at least one other trusted > >> committer before it is allowed to be merged > >> > >> * Require status checks to pass before merging > >> ** pull requests must pass automated checks, for example by being > >> successfully built by a CI service > >> > >> * Restrict who can push to this branch > >> ** official branches (master + stable-x.y) could be restricted to > >> approved committers, but we could grant write access to all other > >> interested developers more easily > >> > >> > >> = Reviews for pull requests = > >> > >> Comments on all commits as well on pull requests have been supported > >> since the early days of GitHub, but more recently it is easier to submit > >> (and request) actual reviews for pull requests. Instead of simply > >> commenting, and changing that special field on Jira, an official pull > >> request status is supported, with three states: > >> > >> - review required > >> - changes requested > >> - approved > >> > >> PR authors can either await for reviews from anybody, or request someone > >> in particular for a review. > >> > >> Committers can filter PRs by their status, and they can also show only > >> the PRs that require their review. > >> > >> > >> = Getting more eyeballs on the code = > >> > >> As Linus' Law states, "given enough eyeballs, all bugs are shallow". > >> We've been experimenting with mandatory pull requests for a while in my > >> other XWiki-based project, with great success so far. So here's a > >> proposal for a different development workflow: > >> > >> 1. Set up
Re: [xwiki-devs] [Proposal] GitHub code reviews and protected branches
Hi Vincent, On 05/17/2017 09:30 AM, Vincent Massol wrote: > Hi Sergiu, > >> On 16 May 2017, at 07:36, Sergiu Dumitriuwrote: >> >> Hi devs, >> >> A while ago GitHub introduced several features that I think can help >> boost even more the code quality, reduce the bus factor, and make it >> more attractive to contribute to the project. >> >> = Protected branches = >> >> Branches can be configured as protected in several ways: >> * Require pull request reviews before merging >> ** no direct commits are allowed, everything must go through a pull request >> ** a pull request must be reviewed by at least one other trusted >> committer before it is allowed to be merged >> >> * Require status checks to pass before merging >> ** pull requests must pass automated checks, for example by being >> successfully built by a CI service >> >> * Restrict who can push to this branch >> ** official branches (master + stable-x.y) could be restricted to >> approved committers, but we could grant write access to all other >> interested developers more easily >> >> >> = Reviews for pull requests = >> >> Comments on all commits as well on pull requests have been supported >> since the early days of GitHub, but more recently it is easier to submit >> (and request) actual reviews for pull requests. Instead of simply >> commenting, and changing that special field on Jira, an official pull >> request status is supported, with three states: >> >> - review required >> - changes requested >> - approved >> >> PR authors can either await for reviews from anybody, or request someone >> in particular for a review. >> >> Committers can filter PRs by their status, and they can also show only >> the PRs that require their review. >> >> >> = Getting more eyeballs on the code = >> >> As Linus' Law states, "given enough eyeballs, all bugs are shallow". >> We've been experimenting with mandatory pull requests for a while in my >> other XWiki-based project, with great success so far. So here's a >> proposal for a different development workflow: >> >> 1. Set up 2 teams, one with master (senior) committers, fully vetted >> committers that have proven they understand the XWiki code, as well as >> the XWiki product, and one with junior committers > > How is that different from what we’re currently doing? > > Right now we have: > * Committers (they’ve been voted committers because they have proven their > knowledge on our platform + long term dedication to the project). They’re the > “senior” of your team > * Contributors, who need to go through PR to get their code in. They’re the > “juniors” of your other team. It's different because: - they are part of the XWiki organization, not just some forkers; people may care about the list of organizations that's displayed on their profile - they can commit code in the official repository, so everybody else can see their commits in one place (including through notification emails), and every other committer can contribute to their branches >> 2. Protect the master branch. Require pull requests and reviews before >> any code is merged in it. Restrict push only to the master committers. > > Main pros for me: > * we can ask for the CI to pass before merging, thus disturbing less other > committers. > * easier to converge for releases but *only* if we’re ready to drop features. > If we’re not then it’s the opposite and makes it harder to converge. > > Main cons: > * takes a LOT of time before code is merged. You can be sure that it’ll need > several hours before it gets merged and that you’ll have moved on to > something else when the comment comes in. And since your new work has an > important chance of being related you might end up chaining the PRs. > >> 3. Everybody works on separate branches > > How does the CI ensure that all branches are merged before the tests are > executed? Why all branches? I'm not sure I understand this... Jenkins has a GitHubPullRequestBuilder plugin, which can build every pull request, after every new push to the PR branch, and this should say if the build will work after merging that particular pull request. It used to be fragile several years ago, but it's working great now. > Example: I’m working on something and I’m modifying some @Unstable API. > Someone else is working with the previous API. We’ll only discover the issue > late when both PRs are merged. There's nothing new, the same thing may happen with everybody committing directly to master. Either there's a conflict, in which case the last pull request to be merged will have to be rebased and reviewed manually, or there's no conflict, but in this case how does committing to master help detect the problem? Other than rebuilding the whole project every time after every pull, in which case someone could be stuck indefinitely in a push->failed->pull->build->repeat loop, as long as others keep pushing while building. That's why the master auto-builds will stay in place, building each
Re: [xwiki-devs] [Proposal] GitHub code reviews and protected branches
Probably my voice is not that important, but even though I'll share my point of view. For me it sounds very interesting. I seems like a chance for getting a good motivation for a very junior, like me, to get deeper into XWiki code and do it in regular mode. So you have - my vote for this idea ;) Best, Krzysztof 2017-05-16 7:36 GMT+02:00 Sergiu Dumitriu: > Hi devs, > > A while ago GitHub introduced several features that I think can help > boost even more the code quality, reduce the bus factor, and make it > more attractive to contribute to the project. > > = Protected branches = > > Branches can be configured as protected in several ways: > * Require pull request reviews before merging > ** no direct commits are allowed, everything must go through a pull request > ** a pull request must be reviewed by at least one other trusted > committer before it is allowed to be merged > > * Require status checks to pass before merging > ** pull requests must pass automated checks, for example by being > successfully built by a CI service > > * Restrict who can push to this branch > ** official branches (master + stable-x.y) could be restricted to > approved committers, but we could grant write access to all other > interested developers more easily > > > = Reviews for pull requests = > > Comments on all commits as well on pull requests have been supported > since the early days of GitHub, but more recently it is easier to submit > (and request) actual reviews for pull requests. Instead of simply > commenting, and changing that special field on Jira, an official pull > request status is supported, with three states: > > - review required > - changes requested > - approved > > PR authors can either await for reviews from anybody, or request someone > in particular for a review. > > Committers can filter PRs by their status, and they can also show only > the PRs that require their review. > > > = Getting more eyeballs on the code = > > As Linus' Law states, "given enough eyeballs, all bugs are shallow". > We've been experimenting with mandatory pull requests for a while in my > other XWiki-based project, with great success so far. So here's a > proposal for a different development workflow: > > 1. Set up 2 teams, one with master (senior) committers, fully vetted > committers that have proven they understand the XWiki code, as well as > the XWiki product, and one with junior committers > 2. Protect the master branch. Require pull requests and reviews before > any code is merged in it. Restrict push only to the master committers. > 3. Everybody works on separate branches > 4. Once the code is almost done, make a pull request and require reviews > from two junior committers > 4a. Or, instead of requesting, let committers volunteer themselves, but > it's less likely that someone will volunteer on time > 5. The junior committers must then carefully review the code, and either > require changes, just comment, or approve the pull request > 6. Once the junior committers have approved the pull request, the PR > author must request review from a senior committer > 7. The senior committer can either approve and merge the pull request, > or send it back to step 3. > e. We can define allowed exceptions: don't require PRs from senior > committers if the fixes are really small and obvious, like fixing typos, > or small and important bugfixes that must be merged quickly, but these > should really be rare exceptions > > While it does sound like more work for an already overworked team, this > has many benefits: > * the code will have better quality > * awareness of: > ** what's new / changing > ** how others are approaching a problem (especially juniors learning > from seniors by being exposed to more code) > ** the existing code, since the codebase is large and otherwise people > have few occasions to look at many of the parts of XWiki > * this means a larger bus factor for new code, and slowly increasing it > for existing code that's being touched by one and reviewed by many > * theoretically, less time spent doing reviews, since all committers > should look over every commit anyway, but this way they are explicitly > told when they should look, instead of wasting time reviewing work in > progress > * larger community, since people can more quickly become junior > committers instead of having to invest many months of years of forkwork > before committership > * easier collaboration on code, since it's very hard to work on someone > else's fork branches, but easy to work on an origin branch > > > So, what do you think? > -- > Sergiu Dumitriu > http://purl.org/net/sergiu/ >
[xwiki-devs] [Proposal] GitHub code reviews and protected branches
Hi devs, A while ago GitHub introduced several features that I think can help boost even more the code quality, reduce the bus factor, and make it more attractive to contribute to the project. = Protected branches = Branches can be configured as protected in several ways: * Require pull request reviews before merging ** no direct commits are allowed, everything must go through a pull request ** a pull request must be reviewed by at least one other trusted committer before it is allowed to be merged * Require status checks to pass before merging ** pull requests must pass automated checks, for example by being successfully built by a CI service * Restrict who can push to this branch ** official branches (master + stable-x.y) could be restricted to approved committers, but we could grant write access to all other interested developers more easily = Reviews for pull requests = Comments on all commits as well on pull requests have been supported since the early days of GitHub, but more recently it is easier to submit (and request) actual reviews for pull requests. Instead of simply commenting, and changing that special field on Jira, an official pull request status is supported, with three states: - review required - changes requested - approved PR authors can either await for reviews from anybody, or request someone in particular for a review. Committers can filter PRs by their status, and they can also show only the PRs that require their review. = Getting more eyeballs on the code = As Linus' Law states, "given enough eyeballs, all bugs are shallow". We've been experimenting with mandatory pull requests for a while in my other XWiki-based project, with great success so far. So here's a proposal for a different development workflow: 1. Set up 2 teams, one with master (senior) committers, fully vetted committers that have proven they understand the XWiki code, as well as the XWiki product, and one with junior committers 2. Protect the master branch. Require pull requests and reviews before any code is merged in it. Restrict push only to the master committers. 3. Everybody works on separate branches 4. Once the code is almost done, make a pull request and require reviews from two junior committers 4a. Or, instead of requesting, let committers volunteer themselves, but it's less likely that someone will volunteer on time 5. The junior committers must then carefully review the code, and either require changes, just comment, or approve the pull request 6. Once the junior committers have approved the pull request, the PR author must request review from a senior committer 7. The senior committer can either approve and merge the pull request, or send it back to step 3. e. We can define allowed exceptions: don't require PRs from senior committers if the fixes are really small and obvious, like fixing typos, or small and important bugfixes that must be merged quickly, but these should really be rare exceptions While it does sound like more work for an already overworked team, this has many benefits: * the code will have better quality * awareness of: ** what's new / changing ** how others are approaching a problem (especially juniors learning from seniors by being exposed to more code) ** the existing code, since the codebase is large and otherwise people have few occasions to look at many of the parts of XWiki * this means a larger bus factor for new code, and slowly increasing it for existing code that's being touched by one and reviewed by many * theoretically, less time spent doing reviews, since all committers should look over every commit anyway, but this way they are explicitly told when they should look, instead of wasting time reviewing work in progress * larger community, since people can more quickly become junior committers instead of having to invest many months of years of forkwork before committership * easier collaboration on code, since it's very hard to work on someone else's fork branches, but easy to work on an origin branch So, what do you think? -- Sergiu Dumitriu http://purl.org/net/sergiu/