Hi all,
in order to promote good development practices, I would like to write a
wiki page stating FreeIPA expectations and helping contributors to
remain on track (or extend the Contribute/Code wiki page).
The topics include a description explaining our use of the tools (such
as github, pagure etc), but also general development habits that
everyone should enforce.
A draft is available below for your comments/suggestions.
Thanks,
Flo
Developing for FreeIPA
1 Opening an issue
* FreeIPA is using pagure.io to track its upstream issues (either corresponding
to bugs or requests for enhancements): https://pagure.io/freeipa/issues
* Bugs against released Fedora version can be logged in Bugzilla
(https://bugzilla.redhat.com/enter_bug.cgi?product=Fedora&component=freeipa)
with the freeipa component.
* Bugs against released Red Hat Enterprise Linux versions of Identity
Management product can be logged in Bugzilla
(https://bugzilla.redhat.com/enter_bug.cgi?product=Red%20Hat%20Enterprise%20Linux%207&component=ipa)
with the ipa component.
When opening an issue, please refer to
https://www.freeipa.org/page/Troubleshooting#Reporting_bugs advices in order to
include the relevant information. The page for new issue
(https://pagure.io/freeipa/new_issue) will display a template guiding you.
2 Working on an issue
When you start working on a pagure.io ticket,
* assign the ticket to yourself by clicking on the "Take" button, in order to
avoid multiple developers working on the same issues.
* follow the Contribute/Code guidelines
(https://www.freeipa.org/page/Contribute/Code).
* create a corresponding test for each ticket
As soon as your code is ready for review:
* create a Pull Request (PR) following
https://www.freeipa.org/page/Pull_request_on_Github
* update the pagure.io ticket: click on "Edit metadata" and edit the on_review
field with the link to the PR
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:
* check if there are tests in ipatests/ which validate your fix
* run these tests using the instructions in https://www.freeipa.org/page/Testing
* list these tests in your PR
* 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).
3 Reviewing a PR
When you start reviewing a PR, add your name to the Assignees list in order to
avoid duplication of effort.
The reviewer responsibilities include the following steps:
* 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.
_______________________________________________
FreeIPA-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
