Jump to: navigation, search



The ironic-core team is responsible for reviewing all changes proposed to repositories (in 'master' branch) that are under the governance of ironic. That includes the following repositories (this list is not exhaustive):

  • openstack/ironic -- the primary services which comprise the OpenStack Ironic project
  • openstack/python-ironicclient -- our Python client library and CLI
  • openstack/ironic-python-agent -- the in-ramdisk Agent which we utilize for inband provisioning of hardware
  • bifrost -- Ansible play books for running Ironic standalone
  • ironic-lib -- common library of functions used by other Ironic projects

Similar to the ironic-core team, the ironic-stable-maint team is responsible for reviewing all changes proposed to the repositories, in any stable (not 'master') branches. There are more restrictive review guidelines for stable branches.

The ironic-python-agent-core team is an additional team which leads the review effort on the below projects. Members of this team may or may not be members of ironic-core.

  • openstack/ironic-python-agent

The ironic-specs-core team is responsible for reviewing design specifications. Not all ironic-core members have the time to dedicate to design reviews (and some would just rather focus on code). Furthermore, some operators routinely contribute to the design process, but don't have the time to regularly review code, and as such they are not members of ironic-core.

  • openstack/ironic-specs

The ironic-inspector-core team is responsible for the following projects:

  • openstack/ironic-inspector

The bifrost-core team is an additional team which leads the review effort on the below projects. Members of this team may or may not be members of ironic-core.

  • openstack/bifrost

All of the teams are not mentioned here. You can find other ironic-related teams (some of them) at:

Note that everyone is encouraged to help review changes, even if you are not a member of one of these teams. All reviews are very useful and are taken into account by the core team members. Participating in the review process is the most critical task on the road to joining any of the teams.

Adding or Removing Members

Each review group above should reflect active participation in the respective team. The ironic-core and ironic-specs-core teams are led by the project PTL, whereas ironic-python-agent-core and ironic-inspector-core are delegated to JayF and dtantsur, respectively.

The addition of new members, or removal of existing members, should be discussed amongst existing cores periodically, and any changes should be paired with an email to the openstack-dev mailing list. Such a discussion may be initiated by any core at any time. A member of the team may be removed at any time by the PTL. This is typically due to a drop off of involvement by the member such that they are no longer meeting expectations to maintain team membership. There is no need for a vote either when adding or removing members, though it is customary to get input from the existing team when making any changes to the team structure.

Note that discussing concerns about a nominee openly can have negative ramifications for the nominee. The effects of being nominated and then publicly -1'd are unpleasant at best and harmful at worst. Therefor, it is preferable to have an initial informal round of discussion amongst the existing core team to ensure that there is enough consensus and desire to work with the nominee, prior to a public vote.

It is also customary to reach out to the nominee privately to confirm their willingness to accept the position prior to any public discussion, though, as one can infer from the review activity an existing level of commitment to the project, this is not necessary.

Membership Expectations

Membership in the core review team is a significant commitment and should not be taken lightly. Maintaining membership on this team takes a lot of time. Further, it is important that the time invested is consistent. It is harmful to the team and the project overall for the core team members' participation to be inconsistent.

Team members are expected to participate in code reviews on a regular (near daily) basis. Members are also expected to stay on top of discussions happening within the project, primarily on the openstack-dev mailing list and IRC. Note that IRC is used quite heavily, so members should be in #openstack-ironic as much as they can. These activities are critical to be able to provide high quality reviews based on the current state of the project that are consistent with the reviews being done by others on the team and consistent with the documented review guidelines.

One metric used to determine the level of participation in reviews is just the number of reviews being done. While there is no hard line for an expectation on the number of reviews you are doing, members are generally expected to be in the same ballpark as the majority of the rest of the team. You can find stats on that here:

The number of reviews is certainly not the only important thing. It is also important that reviews are high quality, such that you gain respect from the other core team members over time. This is done by regularly providing high quality constructive criticism. Your well thought out recommendations for changes are what build credibility for your +1 of a patch.

All of these things are important, but the most important thing of all is to gain trust from the majority of the core team. There's not a well defined list of things you must do to gain this trust, but all of the expectations described here are a starting point.

Review Prioritization

Not all reviews are created equal. Team members should take care in prioritizing where they invest their review time. In general, priority should be based on the priority of the bug or blueprint the patch is associated with. Beyond that, older reviews should get priority. Take a look at the ReviewWorkflowTips page for tips on how to work prioritization into your workflow.

Other notes

  • A -2 is not overridable by any other core, so be very sparing with them. It's a way to signal "this is so bad, I'm going to unilaterally veto it"
  • Avoid the "perception of impropriety"
    • a patch should not be reviewed and approved solely by core team members from the company that proposed the patch (particularly true of large features)
  • Do not approve your own patches
  • Approving a patch requires at least two +2
    • Risky patches should get left up a little longer, so more cores have a chance to review
    • except for trivial patches which can be approved with just one +2. You can use common sense to decide. Since common sense isn't always common, here are some areas where it is OK:
      • fix-ups to address review feedback
      • grammatical/clarification fixes to docstrings and inline comments[1]
      • global requirement (from OpenStack Proposal Bot) updates (depends on the change, but most would probably qualify)
      • translation patches
      • updates to 'development' config files like .gitignore
  • Try to review at least 1 patch per day (on average)
    • and let the team know if you're taking an extended vacation, so they don't wonder why you're not reviewing anything :)

[1] http://lists.openstack.org/pipermail/openstack-dev/2015-February/057794.html