from reviewers for reviewers


The current NetBeans continuous integration (CI) setup has a few aspects which require extra attention from reviewers before PRs should be merged. This page does also list a few tricks to make the review process hopefully a bit easier.

PR approval check list

  • is the PR labeled?
  • did the right tests run? When did they run?
  • is the commit msg and author information (mail, name) correct/descriptive?
  • does the PR title and text still fit to the PR after the Nth iteration?
  • does a refactoring/cleanup obfuscate a bug fix? (suggest to split them into separate commits)
  • (... the obvious things: does this PR improve the project? Is it correct, maintainable, properly squashed etc)

PR merge check list

  • same as above
  • if the PR targets the delivery branch: don't merge, see PRs for delivery
  • am I willing to accept this side quest? If yes → merge

PR triggered CI Jobs (conditional CI pipeline)

PRs trigger CI jobs running as github actions (aka workflows). How many jobs (and steps) are triggered however, is controlled by the PR labels. This makes it very important to label PRs properly!

  • All labels which influence CI contain the [ci] tag in their description which is there to make it easier to find them:
    https://github.com/apache/netbeans/labels?q=%5Bci%5D
     
  • The description mentions what steps or jobs the concrete label is enabling. See link above for examples.

This system is designed to be intuitive and uses basic category labels which were already in use before the conditional CI pipeline was implemented. A PR which improves maven features should have the Maven label on it. The fact that this label is also enabling additional maven tests is just an implementation detail. In most cases it should "just work" without having to think about it.

There are some additional labels available for extra control:

  • ci:all-tests will enable all tests (currently about 10h of CI time), this is useful for large PRs, project wide cleanups and "when in doubt" situations - don't be afraid to use it
  • ci:no-build disables everything (except paperwork job): intended for trivial PRs like readme edits
  • ci:dev-build will upload a dev build zip to the summary page which makes testing PRs more convenient for a wider audience, please keep in mind that this artifact expires (currently 7 days)

The other labels are all basic category labels, for example: Java, Gradle, Platform or PHP.

for more details see comments in: https://github.com/apache/netbeans/blob/master/.github/workflows/main.yml

Merging a PR to master will currently run all tests.

Important:

  • labels are read by the workflow the moment the workflow is triggered. This is why it is important to label a PR before the create button is pressed, otherwise the first workflow run will use the wrong settings
  • non-committers can't label PRs, reviewers must do that for them, the next PR sync will take new labels into account (90% of all PRs need syncs anyway after review)
  • if a PR doesn't have to be synced anymore, a fresh workflow run can be forced via a hidden trigger: Lock the Conversation and unlock it again and it should start on the unlock event.
  • workflow runs by non-committers have to be approved before they run, unfortunately the labels are read at the moment the workflow was created, not when the "approve and run" button is pressed.

Labels are also used to generate the Release Notes

Another reason ALL PRs should be labeled before merge - check before approval please.

Since PRs are directly linked from the release notes, please make sure the PR text is descriptive enough - screenshots are often also well received in case of visual features.

Restarting failed jobs in the pipeline

Individual failed jobs can be restarted as long as the pipeline artifact didn't expire (currently set to 2 days). Once it expired or the workflow was manually canceled the whole workflow has to be restarted from scratch, since only the primary build jobs which run at the start of the pipeline create the artifact.


see https://github.com/apache/netbeans/actions or "Checks" section below the PR to access the workflow run.

Unreliable tests

Unreliable tests can be wrapped in a retry script, a retry will add an annotation to the workflow summary page, for example:

Tips/Tricks

GitHub can generate diff or patch files for you directly from PRs. All you have to do is to append .diff or .patch to the PR url.

Patch files are also useful to quickly check commit headers (emails, names etc) from new contributors but also a time saver for big PRs (grep or diff with NetBeans).


If you have github CLI set up, checking out PRs becomes really easy:

gh pr checkout 4242



  • No labels