Page tree
Skip to end of metadata
Go to start of metadata

Here are listed some checkpoints that OPNFV Functest core developpers should validate before merging:

  • the git commit message must conform with OpenStack Git Commit Good Practice
  • the review must not break the unit test results or hugely decrease the coverage trend -> CI gating 
  • a review fixing a bug should add the related unit tests
  • the review has to comply with traditional Unix permissions
  • the code proposed must comply with OPNFV Functest programming rules
  • the review should target on only one improvement/fix.

It's recommended to vote -1 if one of these requirements is not respected (even for a minor style issue). It helps increasing the quality of our code. 

Git commit message

Its structure must follow this summary mainly extracted from OpenStack Git Commit Good Practice

  • provide a brief description of the change in the first line
  • insert a single blank line after the first line
  • provide a detailed description of the change in the following lines, breaking paragraphs where needed
  • the first line should be limited to 50 characters and should not end with a period
  • subsequent lines should be wrapped at 72 characters (please run $ fold -w 72 -s .git/COMMIT_EDITMSG to check the lines)
  • put the 'JIRA' and 'Change-id' lines at the very end

Please see examples of good practice proposed by OpenStack.

Programming Guidelines

Code proposal must follow Google Python Style Guide and OpenStack Style Guidelines except for the Copyrights which differ.

The next example must be included in every Python header:

# Copyright (c) 2016 Orange and others.
#
# All rights reserved. This program and the accompanying materials
# are made available under the terms of the Apache License, Version 2.0
# which accompanies this distribution, and is available at
# http://www.apache.org/licenses/LICENSE-2.0

Then we rely on PEP 8 as Style Guide. '\' is only allowed for wrapping long 'with' statements (it is not checked by Jenkins).

A special care is required about the next mistakes which we have fixed during last development cycles:

  • sys.exit must not be called outside 'if __name__ == "__main__'
  • we prefer python methods (e.g. those defined in os library) to bash commands called via os.system()

Please run Pylint over your code

Unix permissions

Developing under non-Unix-like Operating Systems or copying files from network shares can break the Unix permissions.

The permission changes should respect the following commands defined in docker/Dockerfile:

 

$ find ${FUNCTEST_REPO_DIR} -name "*.py" \

    -not -path "*tests/unit*" \

    -not -path "*functest_venv*" \

    |xargs grep -L __main__ |cut -d\: -f 1 |xargs chmod -c 644 &&

    find ${FUNCTEST_REPO_DIR} -name "*.sh" \

    -not -path "*functest_venv*" \

    |xargs grep -L \#\! |cut -d\:  -f 1 |xargs chmod -c 644

 

$ find ${FUNCTEST_REPO_DIR} -name "*.py" \

    -not -path "*tests/unit*" \

    -not -path "*functest_venv*" \

    |xargs grep __main__ |cut -d\: -f 1 |xargs chmod -c 755 &&

    find ${FUNCTEST_REPO_DIR} -name "*.sh" \

    -not -path "*functest_venv*" \

Gerrit reviews

Gerrit reviewing_the_change indicates that +1 and -1 levels are just an opinion and +2 and -2 levels are allowing or blocking the change.

0 + comments could be considered as fine as -1 but we recommend to be explicit. Voting -2 is only allowed if

  • the change is considered as false and any new patchset can't help merging
  • another core developper has already accepted the change and the topic has to be discussed

We oblige 2 cores to validate the change. Then the second one must merge the change.

Please take your time and let everyone, core or not, review the code!


  • No labels

7 Comments

  1. Shall we put it together with git tutorial ?

  2. This one is mostly focused on review and what we should check. Both are linked but we can keep them as they are.

    I will add another section about what we must care about (e.g. exit, '\', import, etc...)

  3. Cedric Ollivier What about the rule of minimum 2 +2's to merge?

     

  4. nice list

    • hugely decrease the coverage trend => what hugely means? could we agree that the coverage must not decrease for more than 2% (and implement a check in the jenkins verify job accordingly
    • shall we also indicate here somme sommons regarding the exit values, default constants?

     

    shall we add a section on the best practice for merging

    • 2 * +2
    • no submission/+2/merge by 1 single user (not seen in Functest but seen in associated projects, that broke the production of the docker several days...) => topic could be raised at the TSC
    • role of the contributors ("standard" rules to put -2 (braking the gate, big conceptual changes requiring team consensus), -1 (several rules is described in the first section), 0 (comment), +1, +2)
  5. Yes I add a section about voting. We have already merged some patches with only one guy. But now we are fixing the rules.

    Please let me know if you agree or not the role of the contributors.

     

  6. I would suggest also giving some guideline about how and when to 1/2 

    1. Agree. Please let me know what you would have added in Gerrit reviews section.