Contributing

First of all, thank you for looking to contribute to Saluki! We're excited to have you here.

This document covers some simple guidelines for both filing issues and contributing code to the project.

Filing Issues

If you've discovered a bug, have a feature request, or have discovered a security issue, start by creating a new issue. We use Github issue templates to provide a number of pre-defined issue types, which will prompt you for the necessary information to help us understand the problem.

If none of the existing issue types fit your needs, simply create a Blank issue and provide as much information as you can.

Pull Requests

You've gotten to the point of working on a fix or a new feature, and now you want to contribute that upstream. This is awesome!

We strive for a high level of quality in terms of pull requests, and this guide is meant to help you understand what we expect from a PR so that you're not surprised during the review process.

Keep it small, focused

Avoid changing too many things at once.

For example, if you're working on changes to a component that requires adding new functionality to core types (topology builder, event model, etc), then you should split up your changes into multiple PRs, focusing first on the supporting changes and then the final change that makes the component utilize the new functionality.

Testing

We generally require unit tests for bug fixes, as well as new features.

Sometimes, this can be difficult to do depending on the type of code change: some areas of code don't have a great way to to be run independently and we often choose to not abstract code just to make it testable. This can sometimes require a little more thought and creativity to test the code.

A good rule of thumb to follow is that if you don't have to change the code to make it testable, then you should probably write a test for it. For example, if you're fixing a bug in an existing type, and that type is already being tested in other ways... then the overhead to add an additional test is low, and so you should do it.

Depending on the complexity of the change, we may insist that tests be included, even if it means adding additional abstractions. Reviewers will help guide you on this during the review process.

Commit messages

Please don't be this person: git commit -m "Fixed stuff". Take a moment to write meaningful commit messages.

The commit message should describe the reason for the change and give extra details that will allow someone later on to understand in 5 seconds the thing you've been working on for a day.

This includes editing the commit message generated by Github from:

Including new features

* Fix linter
* WIP
* Add test for x86
* Fix licenses
* Cleanup headers

to:

Including new features

This feature does this and that. Some tests are excluded on x86 because of ...

Pull request workflow

The goals ordered by priority are:

• Make PR reviews (both initial and follow-up reviews) easy for reviewers using GitHub
• On the main branch, have a meaningful commit history that allows understanding (even years later) what each commit does, and why.

You must open the PR when the code is reviewable or you must set the PR as draft if you want to share code before it's ready for actual reviews.

Before the first PR review

Before the first PR review, meaningful commits are best: logically-encapsulated commits help the reviews go quicker and make the job for the reviewer easier. Conflicts with main can be resolved with a git rebase origin/main and a force push if it makes future review(s) easier.

After the first review

After the first review, to make follow-up reviews easier:

• Avoid force pushes: rewriting the history that was already reviewed makes follow-up reviews painful as GitHub loses track of each comment. Instead, address reviews with additional commits on the PR branch.
• Resolve merge conflicts with main using git merge origin/main

How to merge to main

Once reviews are complete, the merge to main should be done with either:

• the squash-merge option, to keep the history of main clean (even though some context/details are lost in the squash). The commit message for this squash should always be edited to concisely describe the commit without extraneous "address review comments" text.
• the rebase-merge option, after manually rewriting the PR’s commit history and force-pushing to the branch. When using this option, the branch must have a clean history.