Fedora PR reviews - meeting notes

# Fedora PR reviews - meeting notes :::info 👉 https://hackmd.io/@python-maint/fedora-pr-process ::: **What PR submitter does** - Ask for review: Paste the PR link to the team Etherpad - Clearly mark the PR as WIP (ideally in the PR title) - Specifies if they need a shorter than normal review (e.g. just sanity check), mention if they have tested the change - Specify if the change should be reviewed commit-by-commit (when there are refactoring commits) - Provides a scratch build if the CI is broken - Make sure the change makes sense & commit is sensible / it's explained well -> everyone in the team should understand the change - Puts the review to a to-review list or asks a specific person, makes sure the PR is not forgotten - If it's possible, the change should contain the tests - if not, tell that is was tested in another way (how) - Open PRs for older branches where relevant. - if it fixes BZ: - set it to ASSIGNED (and *take* it) when you start working on it - put the link to the PR in the BZ once it exists (even WIP), set to POST - if somebody not from the team submits a PR: - we should assume nothing :) - treat is as an idea - say thank you and not demand all the stuff above ^ (don't scare contributors) - credit their work (don't overwrite authorship info) **What PR reviewer does** - Trust but verify! - Read the PR description (and check links in it, e.g. BZ) to get the full context of the change - Check if CI tests passed - Check if specfile Release field should be increased (not necessary for all changes) - Check the changelog and commit messages: does it explain the change well? Try to explain how the change impacts users, rather than just describing the technical change. - Check if the rhbz# is mentioned in the changelog and/or commit message (if there was no release bump or PR is only related to a particular ticket) - If there are one PR per Fedora version, start by reviewing the Rawhide PR - See how it was tested by the submitter and assess whether it's enough or propose/do some other tests - Think about/discuss adding relevant tests to the CI - If something is unclear, ask the PR author for an explanation - Testing outside the scope of the diff: - If there is a change like "replace every occurrence of X" - entire spec file needs to be checked - Reviewer can point out unrelated problems, and discuss with the submitter whether they're in the scope of the PR or should be dealt with separately - What Fedora branches is the PR relevant to? - ask to open PR for the other Fedora releases (so that CI runs on all o'them) - check whether the branches don't diverge unnecessary - ask the submitter for clarifications if so - - When you merge a PR, communicate your intentions wrt (not) building it. **How to test the changes from PR** - Testing can be done by the reviewer, or by the submitter and just verified by the reviewer - Check that the package is installable - Check that the behaviour of the package corresponds to the change - For thoroughness you can test that the previous version of the package is broken, and that the change fixes it - Make the assumption that the PR doesn't fix the issue, and then prove that you are wrong: the RPM fix the issue :-D - First review the code change, then start the testing - For a quick test, the PR (patch) can be tested. But testing the built RPM (scratch build) is the only reliable and reliable test. If you cannot make a full test, explain it in the review or BZ. - (If there is no automated test) manually test the change and explain in the PR/BBZ how you tested the change. - Don't test the change on different Fedora/RHEL version. Don't test in a virtual environment. Try to test in a fresh/clean container/VM. - Assess how big the impact is (backwards incompatible changes...). If spotted: - Reject it in older Fedoras - we're not supposed to introduce such changes there - Communicate it (e.g. on devel and/or python-devel lists or as a Fedora Change) **Copr rebuilds - when & how to evaluate their results** - Submitter should share a link to Copr rebuild (impact check) - When? - it's not a matter of how many packages depend on the changed package - it's about "can my change possibly break/affect some other package?" - if so - do the rebuild - preferably rebuild all the dependant packages - if not sure - ask the team members - - Copr might install the older (original) version of the updated package - you update foo from 3 to 4 - bar BuildRequires `foo < 4`: copr will install foo 3 - Create and link a guide for automated Copr rebuilds - If you have more than 5 failures in your Copr: - create a new "control" Copr repo (without the change) - rebuild only the failed packages from the original testing Copr - automation idea: check Koschei to filter the packages for the control Copr - focus on the packages that fail in your testing Copr (with changes) but succeed in the control Copr (you will eliminate packages that FTBFS) **Commits / code recommendations** - Multiple commits with self-contained changes are better than one commit with several changes - Unrelated small fixes can be added to the PR as long as they're in separate commits - Multiple small commits is easier to review than one big commit. - Use refactoring commits when doing large changes, e.g. - changing indentation of a large chunk of code - automated change (e.g. run `sed` on the code) - put the command used in the PR message - if you need a manual fixup, add *another* commit - Referencing BZs: - `%changelog` (and/or RHEL commit message): - `Resolves: rhbz#000000` <- if this change fixes the bug - `Related: rhbz#000000` <- if this change is only related to the bug - We also use this format in commit messages in RHEL when fighting the checkbz bot - Real links anywhere else, short links recommended: - https://bugzilla.redhat.com/CVE-XXXX - https://bugzilla.redhat.com/000000 **Tips & tricks** - Don't panic - (Quickly) Review your own PRs on Pagure before asking others to review it. - Ask colleagues for help. Always and only bother Miro. He loves to be bothered >_< (joke) - IRC (Libera Chat): #fedora-python and #fedora-devel - Don't let your editor do unrelated changes for you - E.g. removing trailing white space, changing tabs to spaces, etc. (you can commit these separately in a refactoring commit) copr impact check move to: https://hackmd.io/81DXky5lRj-cUiSTeiag2A