Bruce (and others),
As a core reviewer, I’d like to give some comments on “when to post code for review”. I have been seeing a significant number of commits put up for review with zero testing and in some cases, the code is so broken
that it won’t even compile. This is a complete waste of everyone’s time.
For simple changes (this is the vast majority of changes), the author should be completing their testing before posting it for review. This should include:
·
Code compiles successfully.
·
Code can be built into RPMs and an ISO.
·
Tox tests (flake8, py27, etc…) all pass. These can all be run manually and should be run before launching a review.
·
Existing unit tests updated and new unit tests added where applicable.
·
Functional testing done on a live system to ensure code functions correctly. If the author does not know how to test the code they are changing, then they should be asking questions on the mailing list
- before posting the review.
When a review is updated with a new patch set, the above testing needs to be re-done before the new patch set is posted.
I agree that in specific cases, we may want the author to post the code before testing it. But in those cases, the author should immediately do a Workflow -1 and add a comment to the review explaining why it
is being posted without testing. This is not happening, so reviewers have no idea what (if any) testing has been done and are often forced to ask (multiple times) before getting a response from the author.
Our goal should be to make everyone productive (both authors and reviewers). By putting in the upfront effort to test changes before reviews, we will have more productive reviews, less iterations and better quality
code.
Bart
From: Jones, Bruce E [mailto:bruce.e.jones@intel.com]
Sent: October 24, 2018 1:28 PM
To: starlingx-discuss@lists.starlingx.io
Subject: [Starlingx-discuss] Community call Oct 24 notes
Agenda and Notes for the Oct 24th call
o
Our new web page is live!
o
Bug status
§
Only one stx.2018.10 bug still open: https://bugs.launchpad.net/starlingx/+bug/1797234
·
Notes say that the issue is not reproducible, so this does not hold up stx.2018.10 - removing the label and recommending closure
§
A quick review of the new bugs in the last couple of days does not reveal in any issues that would hold up the release
§
Erich to file a new launchpad for the latest build issues on master - potential build blocking issues in the mirror script
·
RT kernel package location - to be discussed over email...
·
https://review.openstack.org/#/c/612822/
o
Testing status
o
Docs status
§
Release note status
o
Backporting policy???
§
Changes that are candidates for backporting should be sent to the [Release] team in email.
§
Release team will review and decide if a backport is required.
o
Release tagging is on hold until EOD today, with a strong desire to get the build changes in first.
o
See https://etherpad.openstack.org/p/stx-2019.03-priorities
o
Strongly encourage sub-projects TL/PLs to put together their lists/plans and update the etherpad asap.
o
https://wiki.openstack.org/wiki/StarlingX/CodeSubmissionGuidelines
o
We should consider clarifying the early review needed to catch design issues between the final code inspection that gates code acceptance
o
The Intel team in particular would benefit from early/often review
o
The goal is to help close the gap on detailed design approaches where the spec may not cover that level of detail, but the developer needs input on the approach before being able to test.
o
There are mechanisms that the developer should use to make it clear that they are requesting early feedback by including WIP or RFC in the first line of the commit message and/or Workflow -1 by the review submitter
o
Core reviewer workload is high but we can offload that to Contributors who are all able to review code.
o
Leverage the virutal environment where possible e.g. in the current Ceph changes that need testing can be done in a virtual duplex config
o
Other changes that require real hardware should be discussed with the Test team.