Contribute/Code

This article should serve both as a reference article for already seasoned FreeIPA developers, but also as a guide for new people who would like to contribute. You can fix an open bug described in chosen Pagure ticket or implement a whole new feature! The steps below should help get you started.

Prepare a feature design using the Feature template and send it for comments to freeipa-devel list. A feedback from experienced FreeIPA developers will help to avoid running into dead ends and also informing the developers about your plans. See currently accepted or proposed designs for inspiration

Understanding FreeIPA

To get started and understand more about the project, you can check the freeipa-workshop. Is a guided tour through the core functionalities and how to setup a FreeIPA environment.

Understanding the source code

To have a better understanding of how the code works (how the pieces fit together, etc), you can read the "Extending FreeIPA Guide" here. It's outdated, however, is still a good source of information.

Update Trac ticket

We use Pagure for work coordination. Please be so kind and assign the ticket you are working on to yourself. It is just a few clicks and it prevents people from working on the same thing at the same time without knowing about each other.

Maybe the ticket does not exist. If you consider the code change small, just send the patch and do not waste time with paperwork. Please create a new ticket if the code change is big (like a new feature, rework of existing component etc.) or it needs a backport for an older branch. It eases coordination, we can discuss different approaches etc.

Comments: keep in mind that your code will be read and modified by other developers. It is important to add meaningful comments that will make your intent clear (the code tells us how but the comment tells us why...)

Changes to requirements

If your change requires a new version of our dependency (like 389-ds-base, pki-ca or httpd), make sure that freeipa.spec.in is updated and patch descriptions justifies a reason for the change. The description helps later in analyzing and tracking requirements differences between FreeIPA versions.

Change to project requirements may also mean a need to regenerate development repositories, so make sure other developers know about it.

Update documentation

Note that if your code change warrants an update in upstream documentation (especially if the related Trac ticket had Affects Documentation flag checked) you are required to update it as well. See Contributing Documentation page for details.

Build

Test

FreeIPA Makefile provides targets allowing to perform basic tests and these tests must be successful:

make fastlint (pycodestyle and pylint)

make fasttest (tests which don't require access to IPA API)

For convenience the 'fastcheck' runs 'fasttest' and 'fastlint' with Python 2 and 3 in one go. A fastcheck takes about half a minute to a minute to execute: make fastcheck. Please see Testing#Fast_test for instructions.

Important rule: For each ticket, a corresponding test must be written.

Create pull request on Github

Create pull request against freeipa/freeipa on Github.
Please follow steps listed here Pull request on Github if you are not sure how to work with pull requests.

Please try to keep commits limited to a single logical change. Multiple commits in the same pull request are preferred because they allow to demonstrate the logic and isolate changes.

Commit message requirements

The pull request must contain a commit message following the template present in the source tree (.git-commit-template):

Explanation must describe the fix or feature + the method chosen to implement it, and can span across multiple lines.

A good commit message allows understanding whether it is related to a new feature, an enhancement, a code refactoring or a bugfix.

It needs to provide a context (for instance the issue happens when command xx is called) and a high-level description of the approach to address the issue, along with potential side-effects.

When a pull request is created, please update Pagure ticket with link to the pull request (in the ticket, click on "Edit Metadata" and update the field "on_review" with the link to your PR, for instance https://github.com/freeipa/freeipa/pull/1234).

The PR will trigger only a subset of the tests. Please keep in mind that, due to resource limitations, all the tests from the source tree will not be executed.

We expect you to check if the PR-CI tests are indeed testing your fix. If some parts of your code are not executed by the PR-CI, then you need to:

mention which commands or scenarios should be thoroughfully checked by the reviewer

describe the manual tests than you have run

A good habit is also to try to reproduce the issue first: build a scenario that consistently shows the issue, then implement the fix, and re-run the same scenario to make sure that the fix is correct.

Once the review is in progress, please remember that the fix is still under your responsibility as long as no ACK has been given. This means that you should answer to questions or requests for modifications and update your PR by taking into accounts all the comments.

If the PR does not progress for a while, you can ask assign the review to a developer by setting an Assignee (in the PR page, click on Assignees and pick a reviewer).

Work through Code Review process

Tracking reviews

PR Submitter responsibilities

A patch may not be merged upstream until it has received an approval - ACK label and passes all mandatory pull request CI checks.

There is an exception to this rule called the "One Liner" rule. When you have a write access to FreeIPA code base and the patch is trivial (e.g. only one line changed), it can be pushed upstream without a review, but it still must pass CI checks. What means trivial? Use your best judgment.

After a pull request is created, FreeIPA developers will take a look at it and report any concerns they have. The developer starting a review of your patch should add his name to Assigned field in the pull request to keep track of the process.

When the reviewer pass a feedback, patch should be then updated to clear all concerns and thus be ready to be merged. See Pull request on Github for advice how to update a pull request. No changes in Pagure are needed when a reviewer rejects the patch or submitter sends a new version of the patch.

PR Reviewer responsibilities

You can also contribute to FreeIPA by reviewing pull requests. When you start reviewing a PR, add your name to the Assignees list in order to avoid duplication of effort.

check that the PR-CI was successful (otherwise comment the PR asking for a fix, for instance because lint failed ...)

check that the PR includes a corresponding test and that the test scenario exhibits the issue

build with the patch

install and run

try to consider how you would have fixed the issue and compare with the proposed fix if a different strategy was picked

try to consider the potential regressions (for instance if a method is modified, identify which parts of the code are using this method, and check whether they are tested)

if the PR-CI does not validate the fix, check if there are existing tests that are relevant and launch them, or perform a manual verification.

Remember that a reviewer also has a teaching or guiding role: suggest enhancements or point to existing portions of code that could be reused, promote good practices and always assume good intent. The PR submitter may be new to FreeIPA or even new to python development and is certainly willing to improve and learn from others.

In the review comments, list the checks that you manually did or the scenario that you tested. Make objective and argumented comments (avoid "I don't like that..." but rather explain "this should not be done this way because...") and suggest improvements or alternate strategies when you request a change.

Finally, when you are OK with the fix, give the ACK label to the PR so that the fix can be pushed by one of the FreeIPA members.

Enjoy the benefits

When your contribution succeeds in the code review, it is pushed to our upstream repository and will be part of our next release. See our Roadmap for details.