diff --git a/README.rst b/README.rst index ef5ba1ddb6..140f56af86 100644 --- a/README.rst +++ b/README.rst @@ -219,57 +219,6 @@ Prepare the branch request by submitting a patch to this repository. jobs, and the validator requires that the patch listed in your deliverable file actually be merged into a proper branch. -Reviewing a Release or Branch Request -===================================== - -Care needs to be taken when reviewing a request such that the version -proposed (1) follows semver rules and (2) will not cause issues between -branches, particularly stable branches (at least stable branches that are not -yet using upper-constraints checking in CI runs, which is anything before -stable/liberty). - -General notes when reviewing a request: - -* Check the version number for SemVer, especially for libraries. -* Make sure the summary of the patch includes the deliverable name and - version number. - -The following rules apply mostly to stable branches and therefore a member of -the `stable-maint-core `_ -team should +1 the following types of changes before they are approved. - -* For libraries, check global-requirements.txt (g-r) in the - `openstack/requirements repo `_ - to make sure the version you are about to release does not cause a - conflict and wedge the gate. Typically this is only a concern on stable - branches with (un)capped dependencies. - - Typical examples of this kind of break (before upper-constraints are used): - - #. A stable branch, for example stable/juno, has uncapped dependencies on a - library and a version is released on a newer branch, e.g. stable/kilo, - and that version has updated requirements from global-requirements in - stable/kilo which conflict with the versions of libraries allowed in - stable/juno. This then leads to ContextualVersionConflict failures when - installing packages on stable/juno. - #. Similar to the point above, but if there are overlapping version ranges - between two branches, like stable/juno and stable/kilo, you can have the - same kinds of issues where a release from one branch which has g-r syncs - specific to that branch gets used in the other branch and things break. - We saw this happen with oslo.utils 1.4.1 which was intended for - stable/juno consumption but because stable/kilo g-r allowed that version, - we broke stable/kilo CI jobs since 1.4.1 had juno-level dependencies. - -* The rule of thumb is that branches should not overlap versions at the minor - version range. For example, stable/juno can require foo>=1.1,<1.2 and - stable/kilo can require foo>=1.2,<1.3. In this way only patch-level versions - are released for foo on stable/juno and stable/kilo. The pin at the minor - version range prevents those patch-level versions from breaking each other's - branch. - -* Ensure that new branches are listed at the end of the branch list in - the file. - Release Approval ================ diff --git a/doc/source/reference/reviewer_guide.rst b/doc/source/reference/reviewer_guide.rst new file mode 100644 index 0000000000..207fcc675d --- /dev/null +++ b/doc/source/reference/reviewer_guide.rst @@ -0,0 +1,211 @@ +================ + Reviewer Guide +================ + +General Review Considerations +============================= + +As you're looking at any given review, you need to keep a few +different things in mind: + +- Where are we in the release cycle? Some rules change depending on + the phase we're in. +- What "release model" does the deliverable being released follow? The + release model sets some general rules for version numbers and + schedule. +- Which branch is the release on? + +Many of the rules tied to these questions are enforced by the +validation job, so when you see an error understanding the rules helps +you understand the error message. + +Approval Policies +================= + +I prioritize the most current series, since that's where most +development work is going to happen. + +For the stable series we have an arrangement with the +`stable-maint-core +`_ team that +if a deliverable has the ``stable:follows-policy`` tag we don't +approve it until they have had a chance to review it. Things that do +not have that governance tag can be approved at any time. + +Releases from master can be approved with a single reviewer. + +Code changes and doc changes and other things like that need 2 +reviewers. + +Releases from someone other than the PTL or liaison must be +acknowledged by one of them with a +1 vote in gerrit. + +Review Checks +============= + +Does the commit message include the name of the deliverable and a +version number? If the request includes multiple deliverables it is OK +to include a team name and date. + +The Validation Report +===================== + +The validation job, ``openstack-tox-validate``, applies the validation +rules that can be automated. It produces a text report in +``tox/validate-request-results.log``. The file contains the output of +what you would see if you ran ``tox -e validate`` for the patch. + +The output is organized based on the rule being enforced. + +We've tried to separate the "debug" output so it is easier to skim for +real content, with the important output left justified. + +Warnings and errors are summarized at the bottom of the file. + +The List Changes Report +======================= + +The ``releases-tox-list-changes`` job produces a text report to +support human reviewers. It writes the report to +``tox/list-changes-results.log``. As with the validate job, it can be +run as ``tox -e list-changes`` locally. + +Reviewers should read this log file for every review. It includes all +of the information that needed to evaluate a release. + +At the top of the file we get the release model, which tells us things +like when releases are allowed, what version numbers are allowed, etc. + +The "team details" section tells us the PTL and Liaison, so we know +who to make sure has acknowledged the request. If one of those people +proposed the patch, we can go ahead without any delay. Otherwise we +want to make sure one of them knows about the release and approves it +so that teams know we aren't going to release things they know are +broken, for example. + +Next the report shows the governance tags for the repository. If the +request is for a release on a stable branch and the project has that +``stable:follows-policy`` tag, there will be a large banner that says +the release needs to be approved by the stable team. Releases from +master will not include the banner, regardless of whether the +deliverable has the tag. + +In the "Details for commit receiving new tag..." section the report +shows what git thinks the previous tag and number of added patches +should be. That's a quick way to verify that we aren't tagging 1.8.0 +after 1.9.0 or something like that. + +The next step shows any other tags already on the commit being tagged. +Sometimes a team will have a 3-part deliverable but only 1 part +changes in a release. If they have defined the 3 parts as 1 +deliverable, they should tag all 3 anyway. + +The next section shows what versions are on all of the branches. This +is somewhat important, since for the first release off of master after +creating a stable branch we want to make sure we are moving ahead in +version numbers. The validation job requires that least the Y value +in a X.Y.Z version number is incremented. + +The next step shows which branch(es) contain(s) the commit. That's +useful for ensuring that someone has not merged 2 branches together +and we are not releasing off of the wrong branch. + +For the current cycle, releases should always come from the ``master`` +branch. Stable releases should come from the appropriate stable +branch. + +The "Relationship to HEAD" section tells us if the release will +skipping any commits. Sometimes someone uses a commit hash locally +that is older than the most recent commit on the branch. If this +section does not say it is releasing HEAD, it is good ask the +submitter to verify that they're doing what they mean to be doing. +Sometimes they don't want to release the additional changes, and +sometimes they don't know about them. It is not necessary to take +this extra precaution for milestone tags, because those are date-based +and it doesn't really matter if they don't include everything. We +expect a lot of churn and progress around the milestone deadlines. + +The next couple of sections show open patches matching various +criteria. These are useful close to the release candidate phase of +the cycle. When we are close to a freeze date the release team might +encourage teams to approve outstanding changes for requirements +updates, release notes, and translations before releasing. + +The next two sections, "Requirements Changes..." and "setup.cfg +Changes...", show the dependencies that have changed for the project +since the last time it was tagged. We use those to ensure that the +exception to the SemVer rules is applied: + +* Projects tagging a regular release (not a "pre-release" like an + alpha, beta, or rc) need to increment at least the Y part of their + version number when they minimum version of a dependency changes or + when a new dependency is added. + +The report shows the changes to the test requirements as the second +part of the "Requirements Changes" section. Those do not trigger Y +version changes. + +The "Release $version will include" section shows the actual changes +being included in the new release -- the difference since the last +version was tagged. This is where the subjective part of the review +really comes in. If a patch release is being tagged and something in +this list looks like a new feature, we want them to tag a minor update +instead. If anything in the list appears to describe a +backwards-incompatible change, we want them to tag a major version +update. + +The next section gives a more detailed view of the log messages. Look +for comments like "delete class X" or "add argument Y to method B" to +indicate the release will not be backwards-compatible. It is not +necessary to *lower* a version number, say if the the release does not +have new features and has only fixed a bug. Sometimes if there is +only one change and it is clearly a bug fix we may ask them to do +that, but most of the time releases include a mix of fixes and +features. + +Another thing to look for is if there are only CI configuration +changes. There is no reason to tag a release if the only change was +to the zuul or tox configuration, because the end user won't see those +changes. That happens sometimes with the projects that have a script +to prepare the release proposal. + +The next part of the output show the same text that will appear in the +release announcement email. It is included so that if building that +text fails for some reason this job will fail and the reno input files +can be fixed instead of having the announce job fail. + +The final part of the output is a list of projects that have the +current deliverable being released in their one of their dependency +lists. That section is useful for evaluating the impact of a late +release when we're in the freeze period. + +Release Jobs +============ + +After a release request merges, the ``tag-releases`` job will start up +in the ``release-post`` pipeline. + +``tag-releases`` reads the file from the releases repository and adds +the tag to the repository mentioned in the deliverable file. + +Adding the tag triggers another job that actually builds the release +and uploads it. + +.. ttx has a nice diagram of that, insert it here + +After a Python package release is uploaded, the job +``propose-upper-constraints`` submits a change to +``openstack/requirements`` to update the ``upper-constraints.txt`` +list. The constraints list is used along with the actual requirements +list to tell the jobs which versions of which packages to +install. Since we maintain that list, every time we release something +that is constrained we want to make sure the value is updated. The +job runs for all python packages, but not all of them are in the +constraint list so sometimes it does not submit a patch. + +Release Job Failures +==================== + +When release jobs fail, messages are sent to release failure mailing +list: +http://lists.openstack.org/cgi-bin/mailman/listinfo/release-job-failures