VyOS Networks Blog

Building an open source network OS for the people, together.

Pull request creation guidelines

Daniil Baturin
Posted 7 May, 2024

Hello, Community!

We are always glad to see new contributors, and we are happy to make an effort to help them get started and feel valued, which is why we make sure that the latest development branch (current) is always trivial to build and offer rewards such as contributor subscriptions to prebuilt LTS release images.

However, we often see first-time contributors get confused about our development process. We have already posted bug/feature request creation guidelines to help users and testers navigate the process — now it's time to talk about pull requests.

Currently, GitHub is our primary source code location, and all contributions to the codebase are made through pull requests. That process is already familiar to people who contribute to other open-source projects, but there are some points specific to VyOS to pay attention to.

Make sure to create a task in Phorge first!

VyOS is a GNU/Linux distribution that naturally consists of multiple packages. For this reason, there is more than one repository, and making the VyOS codebase a mono repo is quite difficult — probably not even desirable.

We have, for example:

  • vyos-build — the build system for the image and CI scripts (Jenkinsfiles) for custom packages we build from source.
  • vyos-1x — the configuration system that ties everything together.
  • vyos-documentation — the reference manual (this one may be integrated with vyos-1x soon, stay tuned!)
  • vyos1x-config — an OCaml library for parsing Vyatta/VyOS 1.x configuration files that now powers a lot of functionality in vyos-1x.
  • libvyosconfig — a wrapper around vyos1x-config that allows Python scripts to use it through the foreign function interface.
  • vyatta-cfg — the legacy config backend.

That's not counting forks of third-party projects where we maintain modifications that are not upstreamed yet and aren't always easy to distribute as modules, such as vyatta-biosdevname.

This fact alone means we cannot use GitHub Issues as our issue tracker — there's no way to track issues across multiple repositories. But there are more reasons: for example, there's no way to track issue status across multiple LTS branches.

That is why we use an external tracker — a Phorge (formerly Phabricator) instance at vyos.dev.

And that is why it's essential to create a task there before you create a pull request. If you don't, there is no way to track its status for the purpose of release notes.

Please make sure that:

  • Your PR title mentions the task, like "firewall: T9999: fix support for matching the evil bit".
  • Your commit messages also mention the task, which may follow the same format as the PR title.

The PR title is just for maintainers to quickly look up the relevant task. Commit messages have an important technical role — if they mention a task number, GitHub/Phorge automatically links them to tasks.

We follow that procedure ourselves, and we will not merge any PR that doesn't follow it — whether it comes from a forgetful maintainer (which happens to all of us sometimes) or from a community contributor.

The only repository where task numbers are not required is vyos-documentation, but if your changes are related to a new feature or similar, please reference the task number as a courtesy.

Make the PR title informative.

Sometimes we see PRs with titles like "Fix firewall". Sometimes it's evident from the code what the fix is for. A lot of the time, it's not apparent at all.

Please ensure that your PR not only references a task number but also briefly describes what exactly you want to change.

These are some good examples:

  • "Fix port number validation in firewall rules"
  • "Add an option to match the evil bit in packets"
  • "Increase the default number of commit revisions"

Make commit messages informative

One problem especially common in the documentation repository is that when people edit it from GitHub's web UI, the default commit message is just "Update somefile.md" — GitHub's default.  I really wish GitHub completely disallowed empty commit messages in the web editor, but for now, I can only ask everyone: please, please, don't do that.

Commit history is supposed to be informative. "Update somefile.md" is not informative at all. Even "fix a typo in the VRRP section" is much better. Something like "Fix incorrect command examples for unicast VRRP" is perfect.

In the code, the commit message should reflect the intent of the change. Especially in glue code that generates configs for other applications, it's often not too obvious why a change is needed. It's a good idea to describe that in the message: succinctly tell what is done in its first line and explain why it was done in the extended message lines.

frobnicator: T9999: specify the crash_randomly option explicitly

In frobd 14.0.0, crash_randomly defaults to true,
which leads to random crashes.
We set it to false to prevent that.

Keep the commit history clean

We often see PRs where the branch includes multiple small commits where the author tried fixing bugs or issues discovered during their own PR work or the review process.

We also often see commits left by merging other branches into the PR branch.

Please don't do that. Sure, we can squash and merge through GitHub's UI, but we keep our own PRs clean, and so should you.

Every logical change must be precisely in one commit, and technical commits must not be in the branch.

It's fine to include multiple commits if they represent distinct changes, for example, if you need to add reusable utility functions to Python libraries to make your main patch work. But every such commit must be a complete change capable of passing the tests.

The post categories:

Comments