Make a Merge Request

Last update: 09 Aug 2024 [History] [Edit]

Testing

Before making any merge request, please make sure you have run the appropriate tests for the release (git branch) which you are targetting. For instance, for the tier-0 release you need to respect the frozen tier 0 policy and run the RunTier0Tests.py script. For all releases you should explain what tests you have run when making the merge request (see below).

Start a merge request

When you push to GitLab, you’ll get several prompts to create a merge request, both at the command line and when you open the project browser. You should ignore these: GitLab will very slow because it will assume you want to merge with main and find a huge number of changes. Fortunately there’s a shortcut to avoid this! You can instead select the “merge requests” button on the left tab:

From here you can create a new merge request

Define the request

A page now opens that allows you to define the merge request. There are a few points you should pay attention to.

  • Check that the Target branch is correct. This is set by default to main, but if your merge is not for the Athena development branch you must change it. (A useful cross check is to see how many Changes are in your merge request - if it’s far more than you expected then the target branch is probably wrong.)

  • The Title and Description are taken from the last commit message of the topic branch. Please edit these to ensure that these fields correctly describe the merge to the person who will review the code
    • it’s your job to convince them that this change is well motivated and well implemented! List the tests that you have run.
    • It is a good convention start the Title with a topic, e.g. MyPackage: ... if your changes only affect one package.

(Here we show a different merge request, but the principle is all the same.)

Before submitting the merge request, you may want to briefly consult the checklist for the review shifters. Make sure that you have taken into account all points to ensure a fast and smooth review process.

Once you are happy with the merge request just press the Submit merge request button.

Warning If you are just doing the tutorial as an exercise then please do not actually submit a dummy merge request, just stop here!

After this happens the merge request page on the main repository will open up:

(Note that that screen shot was taken after the continuous integration system had run - initially many of the fields and labels will be blank.)

Tip By default, all MRs are squashed. You can read more about it here, but the basic idea is the commits you made when developing the MR are compressed into one final commit (and you will might want to edit this commit message, to make sure it is informative and complete). If your commits are well factorised and not too numerous, then you can choose to deselect the ‘squash’ option. However, the final decision about whether to squash or not is the release coordinator, whose job it is to keep the repository commit history clean and comprehensible.

Add labels

ATLAS uses GitLab labels to guide the process of reviewing and handling merge requests. As the developer you have input into this process - by setting appropriate labels on your request you ensure that it is handled correctly by the reviewers and release coordinators.

Note that most labels will just be set automatically by the continuous integration system. Here we list the ones that you might need to set by hand:

urgent urgent label This merge request fixes something that is high priority, it will jump the queue for review
frozen-tier0-violating FT0 label This merge request changes the output from Tier-0 and as such requires special attention from PROC (see the twiki for FT0 policy)
bugfix bugfix label Informs the reviewers and release coordinators that this request fixes an undesired behaviour which may need to be followed up on
full-build fullbuild label The CI machine will clear its CMake build area before and after compiling the MR. This makes the CI take a lot longer to run, but is needed to successfully build some low-level code changes.
full-unit-tests fullunittests label Run the full suite of unit tests for all packages, not just the packages touched by the merge request.
full-integration-tests fullintegrationtests label Run the full suite of integration tests, rather than the subset determined as required for the merge request via a heuristic matching.

Propagating changes (a.k.a. sweeps)

The production branch (currently upstream/24.0) is regularly merged into the main branch (upstream/main) by the release coordinators. They may ask for your help if your merge request to upstream/24.0 is found to conflict with changes which previously went only into upstream/main.

For changes which are needed in other branches a cherry-pick operations should be used to make duplicates of the merge request for the other branch(es). This may be used, for example, to back-port a bug fix to release 21.X branches, or any other legacy branches. This can be achieved via alsoTargetting:XX labels, where XX denotes a target branch.

Warning Don’t use alsoTargetting labels to cherry-pick between the production branch and the main branch. The release coordinator will regularly merge the production branch into the main branch.

ATLAS runs a script that takes accepted merge requests and tries to cherry-pick these changes onto the target branches specified by the labels. If the merge would succeed without a conflict then a new merge request is created (triggering the normal CI and review process) and the label sweep:done is added to the MR. If any requested branches would not then the label sweep:failed is also added and the change should be ported manually.

Tip GitLab provides a Cherry-pick button on merge requests which have been merged. Clicking this is equivalent to adding a sweep label and using the sweeping script.

Revise a merge request

If you need to update a merge request (for example in response to a request from the review shifter), simply push to the same branch again after editing (and testing) the files.

How long or short should a merge request be?

This question has the same answer as “how long is a piece of string?”… it depends on the circumstances.

However, very long merge requests that touch dozens of files and thousands of lines are in general a problem for the following reasons:

  • they are difficult to review by shifters and release coordinators, and the descriptions have to be very detailed and well-written to allow shifters to understand what the code is doing;
  • the discussions on the MR can get very long and confusing;
  • they take longer to approve and so sit around for longer, and this increases the likelihood of merge conflicts;
  • final implementation of whatever project the submitter is working on takes longer;
  • if something goes wrong after merging and we have to revert the MR, we have to revert everything rather than the one problematic part.

In general, MRs should aim to be rather “atomic”, that is, they should only touch a small number of tools/algorithms or configuration blocks - one tool/algorithm per MR might even be preferred. This makes a given MR more digestible and easier to describe and review. Developments that involve touching many different algorithms/tools/config blocks should make use of JIRA tickets (or an epic with associated tickets) which enable the overall development to be described on the ticket and then each MR linked to it.

Developers should not be worried about submitting temporarily “dead code”, that is, new tools/algorithms that won’t be put to use immediately after submission. One should simply write in the MR description something along the lines of

the code in this MR will not be used until X, Y and Z are also implemented as described in the linked JIRA ticket - so this MR will not affect any production or validation workflows until these other steps are completed

These guidelines do not apply to cases where the same small change has been applied in bulk to many files (for example changes to many CMake files at once or some small code quality update). It will be immediately obvious to the shifter that the same change is being applied in many places and the review will be straightforward.

On the other hand, long MRs that result from porting code from (e.g.) release 21 to main should still be broken up into smaller parts, even if the code was reviewed in r21.

Collaborating on large developments

Sometimes it is convenient when collaborating on a development to have all of the code changes in a single large draft MR. This is fine. Once the group of people working on the development have converged and are ready to submit it for review, they should then agree how best to split it up, submit new non-draft MRs, and close the big draft MR. It is very important that such large work-in-progress MRs are kept as draft to avoid premature reviews by shifters.