IAP GITLAB

Skip to content
Snippets Groups Projects

Code Approval Procedure

Last edited by Felix Riehn

If we are in a position where new developments or bug fixes need to be merged from a branch back to master, follow the rules defined here. If there is anything unclear also in specific cases, we can always discuss in our meetings.

In order to review and merge your 'merge requests' (MRs) quickly and efficiently:

  • document and discuss your contributions PRIOR to start to code in an Issue!
  • keep merge request as small as possible.
  • changes limited to one problem/issue/bug.
  • discuss your code contribution in the MR discussion area.
  • make sure coding quality is high, and guidelines are followed.

It is very useful and will speed up the process described here if particular developments have already been discussed in meetings or gitlab Issues BEFORE a MR or any development is started.

A merge request is not complete, if there are no Labels assigned. Labels help the code review.

  • Select between: Bug, Feature, Development
  • Specify, if needed: Critical (must be addressed immediately), Important (blocks other things)
  • Describe the scope: Stack, Documentation, Processes, Build System, Testing, Style, ... (add further items if needed)

In the following graph the typical default workflow is described. It is important that you also read the coding conventions and guidelines:

graph TD;

  subgraph "CORSIKA project"

  X0[Creating new Issue] --> X1[Discuss on gitlab, and/or in meetings, and/or on slack];
  X1 --> X2[Make sure all relevant parts of the discussion are conserved on/copied to the gitlab Issue];
  end

  subgraph "Code contributor"

  X2 --> A000;
  A000[Creating new MR] ==> A001[Via existing gitlab Issue];
  A000 -.-> A002[Via MR/Create new MR];
  A000 -.-> A003[From existing branch, or fork];
  A002 -.-> A004[Make sure the MR title starts with `WIP:` to mark it as 'work in progress'];
  A003 -.-> A004;
  A001 ==> A004;
  style A001 stroke-width:4px;

  A004 --> A005[Use the MR template `New Merge Request`];
  A005 --> A006[If not automatic but possible: create link to Issue `Closes issue #...`];
  A006 --> A007[Assign labels. Check wiki!];

  A007 --> A0

  A0[Working on MR/branch] -- Finished main work --> A2[Make sure all conditions for code review and approval are met];
  A2 -- Ok --> A3[Remove 'WIP' status];
  A3 --> A4[Inform developers/corsika-devel that code review can proceed];
  A3 -.-> A3b[NOTE: this will trigger further scrutiny on gitlab-CI. Make sure your CI jobs still succeed. Trigger job to check, if needed.];
  end

  A4 --> B0[Suited developers picked for code review];
  B0 --> B1[Reviewers are attached to the MR with their name tags];

  B1 --> C1[Reviewer copies task list from below into MR, if not there yet];

  subgraph "Code reviewer"
  C1 --> C2[Set `read for code review` label in MR];
  C2 --> C2c[Reviewer checks each item on task list];
  C2c --> C3[Reviewer discusses with contributor];
  C1 -.-> C2b;
  C2 -.-> C2b[NOTE: this will also trigger the `optional` jobs on gitlab-CI. Make sure the CI jobs still succeed.];
  C2c -.-> C2b;
  C3 -.-> C2b;
  C3 --> C4[If all check marks are done: this means REVIEW IS FINISHED];
  C4 --> C5[Add label `Code review finished`];
  C5 --> C6[Rebase, if needed]
  end

  subgraph "Project owner"
  C6 --> D1[Checks the MR and the review] 
  D1 --> D2[Re-discuss with contributor and reviewer, if needed]
  D1 --> D3[Put MR on agenda of next meeting, if useful]
  D1 ==> D4[MR is merged into master, and MR is closed]
  D2 --> D4
  D3 --> D4
  end

Until all boxes below are checked the reviewer gets back to the contributor and asks for specific fixes. If there are larger issues either the reviewer or the contributor can both discuss this also in CORSIKA meetings or via the corsika-devel mailing list.

Concrete rules are listed here. Make a copy of the below section into the MR discussion, fill in the issue number(s) in Closes #..., and go through all the points:

Closes #....

The code approval procedure is described in the wiki: [Code approval procedure wiki](https://gitlab.ikp.kit.edu/AirShowerPhysics/corsika/-/wikis/Code-Approval-Procedure)

- [ ] The MR is without `WIP/Draft` status
- [ ] Make sure the most recent CI jobs (config, quality, build_test_example) all run fine with no failures
  - if "check clang-format" failed: the code contributor has to run `./do-clang-format.py --apply` eventually with the `--all` option
  - if "check copyright" failed the code contributor has to run`./do-copyright.py --add=20xy` 
- [ ] Make sure also the jobs with MR-label `ready for code review` succeed. This includes the optional jobs, in particular 'coverage', 'release-clang-14", "release-u-22.04" and make sure no problems occur. You may have to trigger a pipeline manually to check this. 
- [ ] Check in the "coverage" job output that the coverage did not decrease. It should always stay, or increase. If it decreased --> ask contributor to add further needed unit tests, and check coverage report. 
- On the MR page, open the "Open in Web IDE" tool
  - [ ] Check if the provided solution solves the Issue, discuss on gitlab
  - [ ] Check that all changes are actually related to the issue
  - [ ] There are no debug statements left, not even commented out
  - [ ] Check all changes for coding rules and guidelines
- When all above is done: 
  - **Add MR label "Code Review Finished"**
  - **Approve** (click button)
  - Rebase, if needed. If unclear: discuss! 
    - [ ] git rebase master (rebase on top of master)
    - [ ] git merge master (merge back changes from master)

Comments

    Please register or sign in to add a comment.

    data privacy - impressum