Post-Retrospective: Code review workflow discussion

Hello everyone,

as an outcome of our sprint retrospective meeting, I'd like to start the discussion about current code-review workflow (with a few exceptions, the general rule is to do reviews after the changes are pushed to the master branch). It has been proposed to move the review process to happen before the changes appear on master and only push them after they have been approved during review. The main arguments for this were:
  - more stable builds (we can enforce that the certain builds pass, before the merge takes place)
  - better quality of the codebase (since every change needs to go through review first, whatever appears on master should already be of high quality - this may help to prevent the "bad code" from spreading)
  - better control over codebase (eg. code freeze can be easily achieved without interfering with other people work)
  - same code review process for everyone (SD & VR & community - direct write access to the repositories is currently granted to the certain people, which means community cannot jump right in and push their changes - they would probably need to go through pull request and have their changes reviewed first before the merge)
There are obvious drawbacks as well, the main one being having to wait longer for the changes to appear on master (and consequently on the demo server). Also if the ticket is blocked by another ticket, you either have to wait for the one's ticket code to go through whole cycle of code review before starting the work on the new one or take incomplete/unreviewed changes and work on them.

Please feel free to share your thoughts about current process and your experiences with working on pre-push reviews, if you have any. Do you see any issues with the current process?

Please note that this is not a request to change the process. This is an opportunity to share the feedback and experiences on code review process workflow.

Best regards,
Sebastian Brudzinski.

Hi again everyone,

I’m glad we are talking about this important stuff. My past experience with “pre-push reviews” is to do a pull request workflow. Any contributors would code on a branch and then make a pull request when they want to submit that to the community. The review process can happen in GitHub rather than Fisheye.

My experience with this is that the process can go very slowly, and it can take days or weeks for other people to really use your code. It also causes lots of re-base or re-write work if it takes a week for the review to finish and then you find out that the master branch has changed since then, so you need to update your work (and then request a re-review!).

So my feeling is that the down-sides of this might out-weigh the advantages for now—at least for core team members like the SolDevelo team members who are all working together on the code base daily. I feel like for that kind of contributor, having direct repository access and making commits to master is allowing everyone to work more quickly, and yes that means things might break, but we notice it quickly and get it fixed quickly.

After the new version of the software has a public, stable release (like 3.0), then changing workflows to a pull request workflow might be worthwhile. Certainly for other contributors around the community (not core team members) this kind of pull request workflow will be much better.

You mentioned about “bad code” going into the product and spreading. I think the best way to deal with that is to give feedback during reviews and have developers go back and improve their code (before their ticket can be marked Done and before we claim those story points as done).

That is my 2 cents on this issue. I look forward to a healthy conversation. I definitely want us to have a process and tools that produce quality code and that helps developers get their jobs done.

-Brandon

···

On 11/16/16, 3:08 AM, "openlmis-dev@googlegroups.com on behalf of Sebastian Brudziński" <openlmis-dev@googlegroups.com on behalf of sbrudzinski@soldevelo.com> wrote:

    Hello everyone,
    
    as an outcome of our sprint retrospective meeting, I'd like to start the
    discussion about current code-review workflow (with a few exceptions,
    the general rule is to do reviews after the changes are pushed to the
    master branch). It has been proposed to move the review process to
    happen before the changes appear on master and only push them after they
    have been approved during review. The main arguments for this were:
      - more stable builds (we can enforce that the certain builds pass,
    before the merge takes place)
      - better quality of the codebase (since every change needs to go
    through review first, whatever appears on master should already be of
    high quality - this may help to prevent the "bad code" from spreading)
      - better control over codebase (eg. code freeze can be easily achieved
    without interfering with other people work)
      - same code review process for everyone (SD & VR & community - direct
    write access to the repositories is currently granted to the certain
    people, which means community cannot jump right in and push their
    changes - they would probably need to go through pull request and have
    their changes reviewed first before the merge)
    There are obvious drawbacks as well, the main one being having to wait
    longer for the changes to appear on master (and consequently on the demo
    server). Also if the ticket is blocked by another ticket, you either
    have to wait for the one's ticket code to go through whole cycle of code
    review before starting the work on the new one or take
    incomplete/unreviewed changes and work on them.
    
    Please feel free to share your thoughts about current process and your
    experiences with working on pre-push reviews, if you have any. Do you
    see any issues with the current process?
    
    Please note that this is not a request to change the process. This is an
    opportunity to share the feedback and experiences on code review process
    workflow.
    
    Best regards,
    Sebastian Brudzinski.

Hi,

I proposed this change (pre-push reviews) on Spring Retrospective because there was presented an issue which demo server on showcase meeting in which there were pushed changes that 'broke' test server.

Main advantage of 'pre-push review' is that only 'good-quality' code is in the master branch but I think we would get nothing on that change if we would only check the code (review) but not check the result of changes (QA). The good example is related with our UI because I can modify some value(s) in CSS, in review everything will look O.K. but in the browser the UI will look bad.

Other (smaller) advantage is that we don't have to remember about 1-hour code freeze before showcase (this was initiated by Pawel in another topic).

Honestly, I don't have problem with our work flow because with direct access to the repository I can push changes, move ticket to review/QA and start working on another ticket. Also, reviewers/QA don't have to do anything extra to get access to my changes.

Moreover if now some reviews takes several days I can only imagine what would happened when we will change our work flow. I think it is good to change that after 3.0 release where more people (from the outside) would add/modify/update the code.

- Lukasz

···

On 11/18/2016 01:16 AM, Brandon Bowersox-Johnson wrote:

Hi again everyone,

I’m glad we are talking about this important stuff. My past experience with “pre-push reviews” is to do a pull request workflow. Any contributors would code on a branch and then make a pull request when they want to submit that to the community. The review process can happen in GitHub rather than Fisheye.

My experience with this is that the process can go very slowly, and it can take days or weeks for other people to really use your code. It also causes lots of re-base or re-write work if it takes a week for the review to finish and then you find out that the master branch has changed since then, so you need to update your work (and then request a re-review!).

So my feeling is that the down-sides of this might out-weigh the advantages for now—at least for core team members like the SolDevelo team members who are all working together on the code base daily. I feel like for that kind of contributor, having direct repository access and making commits to master is allowing everyone to work more quickly, and yes that means things might break, but we notice it quickly and get it fixed quickly.

After the new version of the software has a public, stable release (like 3.0), then changing workflows to a pull request workflow might be worthwhile. Certainly for other contributors around the community (not core team members) this kind of pull request workflow will be much better.

You mentioned about “bad code” going into the product and spreading. I think the best way to deal with that is to give feedback during reviews and have developers go back and improve their code (before their ticket can be marked Done and before we claim those story points as done).

That is my 2 cents on this issue. I look forward to a healthy conversation. I definitely want us to have a process and tools that produce quality code and that helps developers get their jobs done.

-Brandon

On 11/16/16, 3:08 AM, "openlmis-dev@googlegroups.com on behalf of Sebastian Brudziński" <openlmis-dev@googlegroups.com on behalf of sbrudzinski@soldevelo.com> wrote:

    Hello everyone,

    as an outcome of our sprint retrospective meeting, I'd like to start the
    discussion about current code-review workflow (with a few exceptions,
    the general rule is to do reviews after the changes are pushed to the
    master branch). It has been proposed to move the review process to
    happen before the changes appear on master and only push them after they
    have been approved during review. The main arguments for this were:
      - more stable builds (we can enforce that the certain builds pass,
    before the merge takes place)
      - better quality of the codebase (since every change needs to go
    through review first, whatever appears on master should already be of
    high quality - this may help to prevent the "bad code" from spreading)
      - better control over codebase (eg. code freeze can be easily achieved
    without interfering with other people work)
      - same code review process for everyone (SD & VR & community - direct
    write access to the repositories is currently granted to the certain
    people, which means community cannot jump right in and push their
    changes - they would probably need to go through pull request and have
    their changes reviewed first before the merge)
    There are obvious drawbacks as well, the main one being having to wait
    longer for the changes to appear on master (and consequently on the demo
    server). Also if the ticket is blocked by another ticket, you either
    have to wait for the one's ticket code to go through whole cycle of code
    review before starting the work on the new one or take
    incomplete/unreviewed changes and work on them.

    Please feel free to share your thoughts about current process and your
    experiences with working on pre-push reviews, if you have any. Do you
    see any issues with the current process?

    Please note that this is not a request to change the process. This is an
    opportunity to share the feedback and experiences on code review process
    workflow.

    Best regards,
    Sebastian Brudzinski.

Hi again Łukasz and everyone,

It sounds like the broken demo server during showcase is a big pain.

I have an idea about that: We could consider showcasing on the uat.openlmis.org server instead of test.openlmis.org. The UAT server does not automatically build each time there is a commit, instead it only builds when someone pushes the button in Jenkins. So Paweł could push that button 1 or 2 hours before the showcase meeting, then check to make sure it is running well, and the site would not change again. That might be more stable for the showcase.

Paweł and Sebastian would probably want to weigh in on that idea, since the code-freeze idea came from them and they do most of the demo’ing at the showcase. But if there are some simple ways we can solve this pain point that would be great. Thanks,

-Brandon

    Hi,
    
    I proposed this change (pre-push reviews) on Spring Retrospective
    because there was presented an issue which demo server on showcase
    meeting in which there were pushed changes that 'broke' test server.
    
    Main advantage of 'pre-push review' is that only 'good-quality' code is
    in the master branch but I think we would get nothing on that change if
    we would only check the code (review) but not check the result of
    changes (QA). The good example is related with our UI because I can
    modify some value(s) in CSS, in review everything will look O.K. but in
    the browser the UI will look bad.
    
    Other (smaller) advantage is that we don't have to remember about 1-hour
    code freeze before showcase (this was initiated by Pawel in another topic).
    
    Honestly, I don't have problem with our work flow because with direct
    access to the repository I can push changes, move ticket to review/QA
    and start working on another ticket. Also, reviewers/QA don't have to do
    anything extra to get access to my changes.
    
    Moreover if now some reviews takes several days I can only imagine what
    would happened when we will change our work flow. I think it is good to
    change that after 3.0 release where more people (from the outside) would
    add/modify/update the code.
    
    - Lukasz

···

On 11/18/16, 7:10 AM, "openlmis-dev@googlegroups.com on behalf of Łukasz Lewczyński" <openlmis-dev@googlegroups.com on behalf of llewczynski@soldevelo.com> wrote:
    
    On 11/18/2016 01:16 AM, Brandon Bowersox-Johnson wrote:
    > Hi again everyone,
    >
    > I’m glad we are talking about this important stuff. My past experience with “pre-push reviews” is to do a pull request workflow. Any contributors would code on a branch and then make a pull request when they want to submit that to the community. The review process can happen in GitHub rather than Fisheye.
    >
    > My experience with this is that the process can go very slowly, and it can take days or weeks for other people to really use your code. It also causes lots of re-base or re-write work if it takes a week for the review to finish and then you find out that the master branch has changed since then, so you need to update your work (and then request a re-review!).
    >
    > So my feeling is that the down-sides of this might out-weigh the advantages for now—at least for core team members like the SolDevelo team members who are all working together on the code base daily. I feel like for that kind of contributor, having direct repository access and making commits to master is allowing everyone to work more quickly, and yes that means things might break, but we notice it quickly and get it fixed quickly.
    >
    > After the new version of the software has a public, stable release (like 3.0), then changing workflows to a pull request workflow might be worthwhile. Certainly for other contributors around the community (not core team members) this kind of pull request workflow will be much better.
    >
    > You mentioned about “bad code” going into the product and spreading. I think the best way to deal with that is to give feedback during reviews and have developers go back and improve their code (before their ticket can be marked Done and before we claim those story points as done).
    >
    > That is my 2 cents on this issue. I look forward to a healthy conversation. I definitely want us to have a process and tools that produce quality code and that helps developers get their jobs done.
    >
    > -Brandon
    >
    >
    > On 11/16/16, 3:08 AM, "openlmis-dev@googlegroups.com on behalf of Sebastian Brudziński" <openlmis-dev@googlegroups.com on behalf of sbrudzinski@soldevelo.com> wrote:
    >
    > Hello everyone,
    >
    > as an outcome of our sprint retrospective meeting, I'd like to start the
    > discussion about current code-review workflow (with a few exceptions,
    > the general rule is to do reviews after the changes are pushed to the
    > master branch). It has been proposed to move the review process to
    > happen before the changes appear on master and only push them after they
    > have been approved during review. The main arguments for this were:
    > - more stable builds (we can enforce that the certain builds pass,
    > before the merge takes place)
    > - better quality of the codebase (since every change needs to go
    > through review first, whatever appears on master should already be of
    > high quality - this may help to prevent the "bad code" from spreading)
    > - better control over codebase (eg. code freeze can be easily achieved
    > without interfering with other people work)
    > - same code review process for everyone (SD & VR & community - direct
    > write access to the repositories is currently granted to the certain
    > people, which means community cannot jump right in and push their
    > changes - they would probably need to go through pull request and have
    > their changes reviewed first before the merge)
    > There are obvious drawbacks as well, the main one being having to wait
    > longer for the changes to appear on master (and consequently on the demo
    > server). Also if the ticket is blocked by another ticket, you either
    > have to wait for the one's ticket code to go through whole cycle of code
    > review before starting the work on the new one or take
    > incomplete/unreviewed changes and work on them.
    >
    > Please feel free to share your thoughts about current process and your
    > experiences with working on pre-push reviews, if you have any. Do you
    > see any issues with the current process?
    >
    > Please note that this is not a request to change the process. This is an
    > opportunity to share the feedback and experiences on code review process
    > workflow.
    >
    > Best regards,
    > Sebastian Brudzinski.
    >
    >
    
    --
    You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
    To post to this group, send email to openlmis-dev@googlegroups.com.
    To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/9124fe25-886f-d7f1-cfcc-1b57a21ce3b5%40soldevelo.com.
    For more options, visit https://groups.google.com/d/optout.

+1 for showcasing on UAT server.

···

On 18.11.2016 17:27, Brandon Bowersox-Johnson wrote:

Hi again Łukasz and everyone,

It sounds like the broken demo server during showcase is a big pain.

I have an idea about that: We could consider showcasing on the uat.openlmis.org server instead of test.openlmis.org. The UAT server does not automatically build each time there is a commit, instead it only builds when someone pushes the button in Jenkins. So Paweł could push that button 1 or 2 hours before the showcase meeting, then check to make sure it is running well, and the site would not change again. That might be more stable for the showcase.

Paweł and Sebastian would probably want to weigh in on that idea, since the code-freeze idea came from them and they do most of the demo’ing at the showcase. But if there are some simple ways we can solve this pain point that would be great. Thanks,

-Brandon

On 11/18/16, 7:10 AM, "openlmis-dev@googlegroups.com on behalf of Łukasz Lewczyński" <openlmis-dev@googlegroups.com on behalf of llewczynski@soldevelo.com> wrote:

     Hi,
          I proposed this change (pre-push reviews) on Spring Retrospective
     because there was presented an issue which demo server on showcase
     meeting in which there were pushed changes that 'broke' test server.
          Main advantage of 'pre-push review' is that only 'good-quality' code is
     in the master branch but I think we would get nothing on that change if
     we would only check the code (review) but not check the result of
     changes (QA). The good example is related with our UI because I can
     modify some value(s) in CSS, in review everything will look O.K. but in
     the browser the UI will look bad.
          Other (smaller) advantage is that we don't have to remember about 1-hour
     code freeze before showcase (this was initiated by Pawel in another topic).
          Honestly, I don't have problem with our work flow because with direct
     access to the repository I can push changes, move ticket to review/QA
     and start working on another ticket. Also, reviewers/QA don't have to do
     anything extra to get access to my changes.
          Moreover if now some reviews takes several days I can only imagine what
     would happened when we will change our work flow. I think it is good to
     change that after 3.0 release where more people (from the outside) would
     add/modify/update the code.
          - Lukasz
          On 11/18/2016 01:16 AM, Brandon Bowersox-Johnson wrote:
     > Hi again everyone,
     >
     > I’m glad we are talking about this important stuff. My past experience with “pre-push reviews” is to do a pull request workflow. Any contributors would code on a branch and then make a pull request when they want to submit that to the community. The review process can happen in GitHub rather than Fisheye.
     >
     > My experience with this is that the process can go very slowly, and it can take days or weeks for other people to really use your code. It also causes lots of re-base or re-write work if it takes a week for the review to finish and then you find out that the master branch has changed since then, so you need to update your work (and then request a re-review!).
     >
     > So my feeling is that the down-sides of this might out-weigh the advantages for now—at least for core team members like the SolDevelo team members who are all working together on the code base daily. I feel like for that kind of contributor, having direct repository access and making commits to master is allowing everyone to work more quickly, and yes that means things might break, but we notice it quickly and get it fixed quickly.
     >
     > After the new version of the software has a public, stable release (like 3.0), then changing workflows to a pull request workflow might be worthwhile. Certainly for other contributors around the community (not core team members) this kind of pull request workflow will be much better.
     >
     > You mentioned about “bad code” going into the product and spreading. I think the best way to deal with that is to give feedback during reviews and have developers go back and improve their code (before their ticket can be marked Done and before we claim those story points as done).
     >
     > That is my 2 cents on this issue. I look forward to a healthy conversation. I definitely want us to have a process and tools that produce quality code and that helps developers get their jobs done.
     >
     > -Brandon
     >
     > On 11/16/16, 3:08 AM, "openlmis-dev@googlegroups.com on behalf of Sebastian Brudziński" <openlmis-dev@googlegroups.com on behalf of sbrudzinski@soldevelo.com> wrote:
     >
     > Hello everyone,
     >
     > as an outcome of our sprint retrospective meeting, I'd like to start the
     > discussion about current code-review workflow (with a few exceptions,
     > the general rule is to do reviews after the changes are pushed to the
     > master branch). It has been proposed to move the review process to
     > happen before the changes appear on master and only push them after they
     > have been approved during review. The main arguments for this were:
     > - more stable builds (we can enforce that the certain builds pass,
     > before the merge takes place)
     > - better quality of the codebase (since every change needs to go
     > through review first, whatever appears on master should already be of
     > high quality - this may help to prevent the "bad code" from spreading)
     > - better control over codebase (eg. code freeze can be easily achieved
     > without interfering with other people work)
     > - same code review process for everyone (SD & VR & community - direct
     > write access to the repositories is currently granted to the certain
     > people, which means community cannot jump right in and push their
     > changes - they would probably need to go through pull request and have
     > their changes reviewed first before the merge)
     > There are obvious drawbacks as well, the main one being having to wait
     > longer for the changes to appear on master (and consequently on the demo
     > server). Also if the ticket is blocked by another ticket, you either
     > have to wait for the one's ticket code to go through whole cycle of code
     > review before starting the work on the new one or take
     > incomplete/unreviewed changes and work on them.
     >
     > Please feel free to share your thoughts about current process and your
     > experiences with working on pre-push reviews, if you have any. Do you
     > see any issues with the current process?
     >
     > Please note that this is not a request to change the process. This is an
     > opportunity to share the feedback and experiences on code review process
     > workflow.
     >
     > Best regards,
     > Sebastian Brudzinski.
     >
          --
     You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
     To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
     To post to this group, send email to openlmis-dev@googlegroups.com.
     To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/9124fe25-886f-d7f1-cfcc-1b57a21ce3b5%40soldevelo.com.
     For more options, visit https://groups.google.com/d/optout.
     

For me not committing close to the showcase should be enough to solve this issue, but I guess the UAT server is also an acceptable solution. This problem doesn't warrant any larger changes to the workflow in my opinion - I guess some people treated is as the straw that broke the camels back in terms of Fisheye.

In terms of Fisheye - I got used to it to be honest, It's not perfect definitely. I didn't check out the new UI - I guess it's worth looking into. Seems like Fisheye is not kind to moving stuff around and refactoring - hopefully we can limit the number of tasks that involve this going forward.

Regards,
Paweł

···

On 18.11.2016 17:41, Sebastian Brudziński wrote:

+1 for showcasing on UAT server.

On 18.11.2016 17:27, Brandon Bowersox-Johnson wrote:

Hi again Łukasz and everyone,

It sounds like the broken demo server during showcase is a big pain.

I have an idea about that: We could consider showcasing on the uat.openlmis.org server instead of test.openlmis.org. The UAT server does not automatically build each time there is a commit, instead it only builds when someone pushes the button in Jenkins. So Paweł could push that button 1 or 2 hours before the showcase meeting, then check to make sure it is running well, and the site would not change again. That might be more stable for the showcase.

Paweł and Sebastian would probably want to weigh in on that idea, since the code-freeze idea came from them and they do most of the demo’ing at the showcase. But if there are some simple ways we can solve this pain point that would be great. Thanks,

-Brandon

On 11/18/16, 7:10 AM, "openlmis-dev@googlegroups.com on behalf of Łukasz Lewczyński" <openlmis-dev@googlegroups.com on behalf of llewczynski@soldevelo.com> wrote:

     Hi,
          I proposed this change (pre-push reviews) on Spring Retrospective
     because there was presented an issue which demo server on showcase
     meeting in which there were pushed changes that 'broke' test server.
          Main advantage of 'pre-push review' is that only 'good-quality' code is
     in the master branch but I think we would get nothing on that change if
     we would only check the code (review) but not check the result of
     changes (QA). The good example is related with our UI because I can
     modify some value(s) in CSS, in review everything will look O.K. but in
     the browser the UI will look bad.
          Other (smaller) advantage is that we don't have to remember about 1-hour
     code freeze before showcase (this was initiated by Pawel in another topic).
          Honestly, I don't have problem with our work flow because with direct
     access to the repository I can push changes, move ticket to review/QA
     and start working on another ticket. Also, reviewers/QA don't have to do
     anything extra to get access to my changes.
          Moreover if now some reviews takes several days I can only imagine what
     would happened when we will change our work flow. I think it is good to
     change that after 3.0 release where more people (from the outside) would
     add/modify/update the code.
          - Lukasz
          On 11/18/2016 01:16 AM, Brandon Bowersox-Johnson wrote:
     > Hi again everyone,
     >
     > I’m glad we are talking about this important stuff. My past experience with “pre-push reviews” is to do a pull request workflow. Any contributors would code on a branch and then make a pull request when they want to submit that to the community. The review process can happen in GitHub rather than Fisheye.
     >
     > My experience with this is that the process can go very slowly, and it can take days or weeks for other people to really use your code. It also causes lots of re-base or re-write work if it takes a week for the review to finish and then you find out that the master branch has changed since then, so you need to update your work (and then request a re-review!).
     >
     > So my feeling is that the down-sides of this might out-weigh the advantages for now—at least for core team members like the SolDevelo team members who are all working together on the code base daily. I feel like for that kind of contributor, having direct repository access and making commits to master is allowing everyone to work more quickly, and yes that means things might break, but we notice it quickly and get it fixed quickly.
     >
     > After the new version of the software has a public, stable release (like 3.0), then changing workflows to a pull request workflow might be worthwhile. Certainly for other contributors around the community (not core team members) this kind of pull request workflow will be much better.
     >
     > You mentioned about “bad code” going into the product and spreading. I think the best way to deal with that is to give feedback during reviews and have developers go back and improve their code (before their ticket can be marked Done and before we claim those story points as done).
     >
     > That is my 2 cents on this issue. I look forward to a healthy conversation. I definitely want us to have a process and tools that produce quality code and that helps developers get their jobs done.
     >
     > -Brandon
     >
     > On 11/16/16, 3:08 AM, "openlmis-dev@googlegroups.com on behalf of Sebastian Brudziński" <openlmis-dev@googlegroups.com on behalf of sbrudzinski@soldevelo.com> wrote:
     >
     > Hello everyone,
     >
     > as an outcome of our sprint retrospective meeting, I'd like to start the
     > discussion about current code-review workflow (with a few exceptions,
     > the general rule is to do reviews after the changes are pushed to the
     > master branch). It has been proposed to move the review process to
     > happen before the changes appear on master and only push them after they
     > have been approved during review. The main arguments for this were:
     > - more stable builds (we can enforce that the certain builds pass,
     > before the merge takes place)
     > - better quality of the codebase (since every change needs to go
     > through review first, whatever appears on master should already be of
     > high quality - this may help to prevent the "bad code" from spreading)
     > - better control over codebase (eg. code freeze can be easily achieved
     > without interfering with other people work)
     > - same code review process for everyone (SD & VR & community - direct
     > write access to the repositories is currently granted to the certain
     > people, which means community cannot jump right in and push their
     > changes - they would probably need to go through pull request and have
     > their changes reviewed first before the merge)
     > There are obvious drawbacks as well, the main one being having to wait
     > longer for the changes to appear on master (and consequently on the demo
     > server). Also if the ticket is blocked by another ticket, you either
     > have to wait for the one's ticket code to go through whole cycle of code
     > review before starting the work on the new one or take
     > incomplete/unreviewed changes and work on them.
     >
     > Please feel free to share your thoughts about current process and your
     > experiences with working on pre-push reviews, if you have any. Do you
     > see any issues with the current process?
     >
     > Please note that this is not a request to change the process. This is an
     > opportunity to share the feedback and experiences on code review process
     > workflow.
     >
     > Best regards,
     > Sebastian Brudzinski.
     >
          --
     You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
     To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
     To post to this group, send email to openlmis-dev@googlegroups.com.
     To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/9124fe25-886f-d7f1-cfcc-1b57a21ce3b5%40soldevelo.com.
     For more options, visit https://groups.google.com/d/optout.

Hey everyone,

UAT sounds good, although I thought it would only have the beta code for now, so that anyone could see what beta looks like?

Shalom,
Chongsun

-- ​
There are 10 kinds of people in this world; those who understand binary, and those who don’t.

···

On Nov 18, 2016, at 8:41 AM, Sebastian Brudziński <sbrudzinski@soldevelo.com> wrote:

+1 for showcasing on UAT server.

On 18.11.2016 17:27, Brandon Bowersox-Johnson wrote:
Hi again Łukasz and everyone,

It sounds like the broken demo server during showcase is a big pain.

I have an idea about that: We could consider showcasing on the uat.openlmis.org server instead of test.openlmis.org. The UAT server does not automatically build each time there is a commit, instead it only builds when someone pushes the button in Jenkins. So Paweł could push that button 1 or 2 hours before the showcase meeting, then check to make sure it is running well, and the site would not change again. That might be more stable for the showcase.

Paweł and Sebastian would probably want to weigh in on that idea, since the code-freeze idea came from them and they do most of the demo’ing at the showcase. But if there are some simple ways we can solve this pain point that would be great. Thanks,

-Brandon

On 11/18/16, 7:10 AM, "openlmis-dev@googlegroups.com on behalf of Łukasz Lewczyński" <openlmis-dev@googlegroups.com on behalf of llewczynski@soldevelo.com> wrote:

    Hi,
         I proposed this change (pre-push reviews) on Spring Retrospective
    because there was presented an issue which demo server on showcase
    meeting in which there were pushed changes that 'broke' test server.
         Main advantage of 'pre-push review' is that only 'good-quality' code is
    in the master branch but I think we would get nothing on that change if
    we would only check the code (review) but not check the result of
    changes (QA). The good example is related with our UI because I can
    modify some value(s) in CSS, in review everything will look O.K. but in
    the browser the UI will look bad.
         Other (smaller) advantage is that we don't have to remember about 1-hour
    code freeze before showcase (this was initiated by Pawel in another topic).
         Honestly, I don't have problem with our work flow because with direct
    access to the repository I can push changes, move ticket to review/QA
    and start working on another ticket. Also, reviewers/QA don't have to do
    anything extra to get access to my changes.
         Moreover if now some reviews takes several days I can only imagine what
    would happened when we will change our work flow. I think it is good to
    change that after 3.0 release where more people (from the outside) would
    add/modify/update the code.
         - Lukasz
         On 11/18/2016 01:16 AM, Brandon Bowersox-Johnson wrote:
    > Hi again everyone,
    >
    > I’m glad we are talking about this important stuff. My past experience with “pre-push reviews” is to do a pull request workflow. Any contributors would code on a branch and then make a pull request when they want to submit that to the community. The review process can happen in GitHub rather than Fisheye.
    >
    > My experience with this is that the process can go very slowly, and it can take days or weeks for other people to really use your code. It also causes lots of re-base or re-write work if it takes a week for the review to finish and then you find out that the master branch has changed since then, so you need to update your work (and then request a re-review!).
    >
    > So my feeling is that the down-sides of this might out-weigh the advantages for now—at least for core team members like the SolDevelo team members who are all working together on the code base daily. I feel like for that kind of contributor, having direct repository access and making commits to master is allowing everyone to work more quickly, and yes that means things might break, but we notice it quickly and get it fixed quickly.
    >
    > After the new version of the software has a public, stable release (like 3.0), then changing workflows to a pull request workflow might be worthwhile. Certainly for other contributors around the community (not core team members) this kind of pull request workflow will be much better.
    >
    > You mentioned about “bad code” going into the product and spreading. I think the best way to deal with that is to give feedback during reviews and have developers go back and improve their code (before their ticket can be marked Done and before we claim those story points as done).
    >
    > That is my 2 cents on this issue. I look forward to a healthy conversation. I definitely want us to have a process and tools that produce quality code and that helps developers get their jobs done.
    >
    > -Brandon
    >
    >
    > On 11/16/16, 3:08 AM, "openlmis-dev@googlegroups.com on behalf of Sebastian Brudziński" <openlmis-dev@googlegroups.com on behalf of sbrudzinski@soldevelo.com> wrote:
    >
    > Hello everyone,
    >
    > as an outcome of our sprint retrospective meeting, I'd like to start the
    > discussion about current code-review workflow (with a few exceptions,
    > the general rule is to do reviews after the changes are pushed to the
    > master branch). It has been proposed to move the review process to
    > happen before the changes appear on master and only push them after they
    > have been approved during review. The main arguments for this were:
    > - more stable builds (we can enforce that the certain builds pass,
    > before the merge takes place)
    > - better quality of the codebase (since every change needs to go
    > through review first, whatever appears on master should already be of
    > high quality - this may help to prevent the "bad code" from spreading)
    > - better control over codebase (eg. code freeze can be easily achieved
    > without interfering with other people work)
    > - same code review process for everyone (SD & VR & community - direct
    > write access to the repositories is currently granted to the certain
    > people, which means community cannot jump right in and push their
    > changes - they would probably need to go through pull request and have
    > their changes reviewed first before the merge)
    > There are obvious drawbacks as well, the main one being having to wait
    > longer for the changes to appear on master (and consequently on the demo
    > server). Also if the ticket is blocked by another ticket, you either
    > have to wait for the one's ticket code to go through whole cycle of code
    > review before starting the work on the new one or take
    > incomplete/unreviewed changes and work on them.
    >
    > Please feel free to share your thoughts about current process and your
    > experiences with working on pre-push reviews, if you have any. Do you
    > see any issues with the current process?
    >
    > Please note that this is not a request to change the process. This is an
    > opportunity to share the feedback and experiences on code review process
    > workflow.
    >
    > Best regards,
    > Sebastian Brudzinski.
    >
    >
         --
    You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
    To post to this group, send email to openlmis-dev@googlegroups.com.
    To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/9124fe25-886f-d7f1-cfcc-1b57a21ce3b5%40soldevelo.com.
    For more options, visit https://groups.google.com/d/optout.
    
--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
To post to this group, send email to openlmis-dev@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/582F2F33.9000501%40soldevelo.com.
For more options, visit https://groups.google.com/d/optout.

Folks,
If we decide to use UAT, I can update the community. No one is actively using the UAT code to view the beta code. Initially folks took a look but it isn't needed long term. I think we can just point to the beta code on github. However, I would want to know so I can communicate the update.

Regards,
Mary Jo

···

-----Original Message-----
From: openlmis-dev@googlegroups.com [mailto:openlmis-dev@googlegroups.com] On Behalf Of Chongsun Ahn
Sent: Friday, November 18, 2016 7:48 PM
To: Sebastian Brudziński <sbrudzinski@soldevelo.com>
Cc: openlmis-dev@googlegroups.com
Subject: Re: [openlmis-dev] Post-Retrospective: Code review workflow discussion

Hey everyone,

UAT sounds good, although I thought it would only have the beta code for now, so that anyone could see what beta looks like?

Shalom,
Chongsun

-- ​
There are 10 kinds of people in this world; those who understand binary, and those who don’t.

On Nov 18, 2016, at 8:41 AM, Sebastian Brudziński <sbrudzinski@soldevelo.com> wrote:

+1 for showcasing on UAT server.

On 18.11.2016 17:27, Brandon Bowersox-Johnson wrote:
Hi again Łukasz and everyone,

It sounds like the broken demo server during showcase is a big pain.

I have an idea about that: We could consider showcasing on the uat.openlmis.org server instead of test.openlmis.org. The UAT server does not automatically build each time there is a commit, instead it only builds when someone pushes the button in Jenkins. So Paweł could push that button 1 or 2 hours before the showcase meeting, then check to make sure it is running well, and the site would not change again. That might be more stable for the showcase.

Paweł and Sebastian would probably want to weigh in on that idea, since the code-freeze idea came from them and they do most of the demo’ing at the showcase. But if there are some simple ways we can solve this pain point that would be great. Thanks,

-Brandon

On 11/18/16, 7:10 AM, "openlmis-dev@googlegroups.com on behalf of Łukasz Lewczyński" <openlmis-dev@googlegroups.com on behalf of llewczynski@soldevelo.com> wrote:

    Hi,
         I proposed this change (pre-push reviews) on Spring Retrospective
    because there was presented an issue which demo server on showcase
    meeting in which there were pushed changes that 'broke' test server.
         Main advantage of 'pre-push review' is that only 'good-quality' code is
    in the master branch but I think we would get nothing on that change if
    we would only check the code (review) but not check the result of
    changes (QA). The good example is related with our UI because I can
    modify some value(s) in CSS, in review everything will look O.K. but in
    the browser the UI will look bad.
         Other (smaller) advantage is that we don't have to remember about 1-hour
    code freeze before showcase (this was initiated by Pawel in another topic).
         Honestly, I don't have problem with our work flow because with direct
    access to the repository I can push changes, move ticket to review/QA
    and start working on another ticket. Also, reviewers/QA don't have to do
    anything extra to get access to my changes.
         Moreover if now some reviews takes several days I can only imagine what
    would happened when we will change our work flow. I think it is good to
    change that after 3.0 release where more people (from the outside) would
    add/modify/update the code.
         - Lukasz
         On 11/18/2016 01:16 AM, Brandon Bowersox-Johnson wrote:
    > Hi again everyone,
    >
    > I’m glad we are talking about this important stuff. My past experience with “pre-push reviews” is to do a pull request workflow. Any contributors would code on a branch and then make a pull request when they want to submit that to the community. The review process can happen in GitHub rather than Fisheye.
    >
    > My experience with this is that the process can go very slowly, and it can take days or weeks for other people to really use your code. It also causes lots of re-base or re-write work if it takes a week for the review to finish and then you find out that the master branch has changed since then, so you need to update your work (and then request a re-review!).
    >
    > So my feeling is that the down-sides of this might out-weigh the advantages for now—at least for core team members like the SolDevelo team members who are all working together on the code base daily. I feel like for that kind of contributor, having direct repository access and making commits to master is allowing everyone to work more quickly, and yes that means things might break, but we notice it quickly and get it fixed quickly.
    >
    > After the new version of the software has a public, stable release (like 3.0), then changing workflows to a pull request workflow might be worthwhile. Certainly for other contributors around the community (not core team members) this kind of pull request workflow will be much better.
    >
    > You mentioned about “bad code” going into the product and spreading. I think the best way to deal with that is to give feedback during reviews and have developers go back and improve their code (before their ticket can be marked Done and before we claim those story points as done).
    >
    > That is my 2 cents on this issue. I look forward to a healthy conversation. I definitely want us to have a process and tools that produce quality code and that helps developers get their jobs done.
    >
    > -Brandon
    >
    >
    > On 11/16/16, 3:08 AM, "openlmis-dev@googlegroups.com on behalf of Sebastian Brudziński" <openlmis-dev@googlegroups.com on behalf of sbrudzinski@soldevelo.com> wrote:
    >
    > Hello everyone,
    >
    > as an outcome of our sprint retrospective meeting, I'd like to start the
    > discussion about current code-review workflow (with a few exceptions,
    > the general rule is to do reviews after the changes are pushed to the
    > master branch). It has been proposed to move the review process to
    > happen before the changes appear on master and only push them after they
    > have been approved during review. The main arguments for this were:
    > - more stable builds (we can enforce that the certain builds pass,
    > before the merge takes place)
    > - better quality of the codebase (since every change needs to go
    > through review first, whatever appears on master should already be of
    > high quality - this may help to prevent the "bad code" from spreading)
    > - better control over codebase (eg. code freeze can be easily achieved
    > without interfering with other people work)
    > - same code review process for everyone (SD & VR & community - direct
    > write access to the repositories is currently granted to the certain
    > people, which means community cannot jump right in and push their
    > changes - they would probably need to go through pull request and have
    > their changes reviewed first before the merge)
    > There are obvious drawbacks as well, the main one being having to wait
    > longer for the changes to appear on master (and consequently on the demo
    > server). Also if the ticket is blocked by another ticket, you either
    > have to wait for the one's ticket code to go through whole cycle of code
    > review before starting the work on the new one or take
    > incomplete/unreviewed changes and work on them.
    >
    > Please feel free to share your thoughts about current process and your
    > experiences with working on pre-push reviews, if you have any. Do you
    > see any issues with the current process?
    >
    > Please note that this is not a request to change the process. This is an
    > opportunity to share the feedback and experiences on code review process
    > workflow.
    >
    > Best regards,
    > Sebastian Brudzinski.
    >
    >
         --
    You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
    To post to this group, send email to openlmis-dev@googlegroups.com.
    To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/9124fe25-886f-d7f1-cfcc-1b57a21ce3b5%40soldevelo.com.
    For more options, visit https://groups.google.com/d/optout.
    
--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
To post to this group, send email to openlmis-dev@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/582F2F33.9000501%40soldevelo.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "OpenLMIS Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to openlmis-dev+unsubscribe@googlegroups.com.
To post to this group, send email to openlmis-dev@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/openlmis-dev/3F46C40C-04E4-4C0C-A2AC-8039E977890A%40villagereach.org.
For more options, visit https://groups.google.com/d/optout.

I also agree that using the UAT server for showcase is a good idea.

Also I second Brandon’s thoughts on pull-request based workflow’s being cumbersome and that we should be constantly curbing technical debt (bad code) through code review. I’d also like us to think about the tooling we have already in CI pipelines and types of testing. We’ve set these up so that Jenkins can do some of the heavy lifting for us, such as running Contract tests. We’ve started down this path as we know that it will only get harder for an individual developer to be able to compile and run all of the different Services and Extension Modules on their own box for every change. The sooner we get our code noticed by Jenkins therefore, the sooner we will know if something, somewhere, doesn’t work - and knowing this early on is a very good thing.

Of course, this is reliant on writing good tests - unit, integration, component and of course contract. We all write bad code. Or at least I know I do. I know I write better code if I plan the basics out, I focus on writing useful tests, I have someone with a critical eye in code-review challenge me (especially on structure and tests), and am not afraid to refactor something later since there will be tests to guide me.

Pull-requests can be good, but lets not think that they solve everything. A one-time check can never guarantee that everything is good nor that it would stay good. A set of well written tests can deliver value long after they’re run the first time. So code-reviewers, don’t forget to review those tests!

Best,
Josh

···

On Wednesday, November 16, 2016 at 3:08:49 AM UTC-8, Sebastian Brudziński wrote:

Hello everyone,

as an outcome of our sprint retrospective meeting, I’d like to start the
discussion about current code-review workflow (with a few exceptions,
the general rule is to do reviews after the changes are pushed to the
master branch). It has been proposed to move the review process to
happen before the changes appear on master and only push them after they
have been approved during review. The main arguments for this were:

  • more stable builds (we can enforce that the certain builds pass,
    before the merge takes place)
  • better quality of the codebase (since every change needs to go
    through review first, whatever appears on master should already be of
    high quality - this may help to prevent the “bad code” from spreading)
  • better control over codebase (eg. code freeze can be easily achieved
    without interfering with other people work)
  • same code review process for everyone (SD & VR & community - direct
    write access to the repositories is currently granted to the certain
    people, which means community cannot jump right in and push their
    changes - they would probably need to go through pull request and have
    their changes reviewed first before the merge)
    There are obvious drawbacks as well, the main one being having to wait
    longer for the changes to appear on master (and consequently on the demo
    server). Also if the ticket is blocked by another ticket, you either
    have to wait for the one’s ticket code to go through whole cycle of code
    review before starting the work on the new one or take
    incomplete/unreviewed changes and work on them.

Please feel free to share your thoughts about current process and your
experiences with working on pre-push reviews, if you have any. Do you
see any issues with the current process?

Please note that this is not a request to change the process. This is an
opportunity to share the feedback and experiences on code review process
workflow.

Best regards,
Sebastian Brudzinski.

While I definitely agree that we need good tests and should always pursue towards improving them, I don’t think they are always the answer to everything. The main role of unit, integration, contract testing is to give us a simple answer whether something works or not. They don’t really tell us much about the code quality itself (although they can definitely help in improving it, when eg. used to help in refactors, like in your example, Josh). So, what I’m trying to say is that in my opinion good tests are just a small piece of the code quality management and that they, themselves, cannot ensure that the code will be easy to read, maintain, not to complex etc. Anyways, those are just my thoughts about the code quality itself.

Concerning bad code - yes, of course this is meant to be fixed during code review. The issue is that before the bad code gets fixed it lives for some time in the codebase (hopefully not long with quick reviews) and during that time the bad code may spread. A lot of people treat the code base as a point of reference, because honestly, if something is already there and works - why not do my thing the similar way? Maybe we should be more judgemental in general, when looking at our codebase (we definitely should), but I feel that the “code spread”, whether it is good or bad is a natural thing. I can see pros & cons of both approaches and I’m happy to hear about them from everyone.

For the UAT server for showcase - if there are no objections, I’ll deploy the services there like an hour or two before the meeting and we will attempt to showcase from UAT this Wednesday.

Thanks for all the posts!

Kind regards,
Sebastian Brudzinski.

···

On Monday, November 21, 2016 at 8:53:15 AM UTC+1, Josh Zamor wrote:

I also agree that using the UAT server for showcase is a good idea.

Also I second Brandon’s thoughts on pull-request based workflow’s being cumbersome and that we should be constantly curbing technical debt (bad code) through code review. I’d also like us to think about the tooling we have already in CI pipelines and types of testing. We’ve set these up so that Jenkins can do some of the heavy lifting for us, such as running Contract tests. We’ve started down this path as we know that it will only get harder for an individual developer to be able to compile and run all of the different Services and Extension Modules on their own box for every change. The sooner we get our code noticed by Jenkins therefore, the sooner we will know if something, somewhere, doesn’t work - and knowing this early on is a very good thing.

Of course, this is reliant on writing good tests - unit, integration, component and of course contract. We all write bad code. Or at least I know I do. I know I write better code if I plan the basics out, I focus on writing useful tests, I have someone with a critical eye in code-review challenge me (especially on structure and tests), and am not afraid to refactor something later since there will be tests to guide me.

Pull-requests can be good, but lets not think that they solve everything. A one-time check can never guarantee that everything is good nor that it would stay good. A set of well written tests can deliver value long after they’re run the first time. So code-reviewers, don’t forget to review those tests!

Best,
Josh

On Wednesday, November 16, 2016 at 3:08:49 AM UTC-8, Sebastian Brudziński wrote:

Hello everyone,

as an outcome of our sprint retrospective meeting, I’d like to start the
discussion about current code-review workflow (with a few exceptions,
the general rule is to do reviews after the changes are pushed to the
master branch). It has been proposed to move the review process to
happen before the changes appear on master and only push them after they
have been approved during review. The main arguments for this were:

  • more stable builds (we can enforce that the certain builds pass,
    before the merge takes place)
  • better quality of the codebase (since every change needs to go
    through review first, whatever appears on master should already be of
    high quality - this may help to prevent the “bad code” from spreading)
  • better control over codebase (eg. code freeze can be easily achieved
    without interfering with other people work)
  • same code review process for everyone (SD & VR & community - direct
    write access to the repositories is currently granted to the certain
    people, which means community cannot jump right in and push their
    changes - they would probably need to go through pull request and have
    their changes reviewed first before the merge)
    There are obvious drawbacks as well, the main one being having to wait
    longer for the changes to appear on master (and consequently on the demo
    server). Also if the ticket is blocked by another ticket, you either
    have to wait for the one’s ticket code to go through whole cycle of code
    review before starting the work on the new one or take
    incomplete/unreviewed changes and work on them.

Please feel free to share your thoughts about current process and your
experiences with working on pre-push reviews, if you have any. Do you
see any issues with the current process?

Please note that this is not a request to change the process. This is an
opportunity to share the feedback and experiences on code review process
workflow.

Best regards,
Sebastian Brudzinski.