The Drud project has a number of development norms which we try to state explicitly when we can:
- Use PRs to contribute: Even when contributors have the privilege to directly commit to a repo, they typically contribute via a PR instead, which allows everybody to review and be aware of changes, even when they're very small. (Github docs)
- Fork-and-branch PRs: We typically use the fork-and-branch PR technique, where every contributor has their own fork of the main repo, and branches on the fork are used for PRs. This allows the contributor full control of their own repo. (Github docs)
- Squash Merging: By default we use the "squash merging" technique when merging pull requests. This keeps our git commit history clean, and groups all commits from a pull request into a single commit, which is easier to track and roll back, if needed. It's preferred the "Allow merge commits" option in GitHub is disabled for all repositories.
- Provisional Tags: If a pull request involves updating a container then a provisional tag for that feature should be pushed to the containers repostiroy in the DRUD organization on dockerhub. The pull request should use this provisional tag. This helps to ensure automated testing is run against the updated container, and ensures reviewers are testing with the updated asset as well.
- PR Naming (and commits): Please name PRs so that people can (mostly) understand what they are about by just reading the title. Remember that people don't have time to click through every link, and you want them to know what's going on with a one-liner where possible. So "Restart ddev router after rebuilding db container, fixes #998" instead of "Restart router", for example. When possible, it's best for commits to have "real" summary/message as well, not "fix bug". We all know there are experimental times when this becomes difficult.
- PR Review is required: With few exceptions, PRs require review before they are pulled. Significant PRs require review by at least two team members. These guidelines are generally followed even if the repo is not fully set up to enforce them. Being a reviewer for a pull request you worked on is discouraged.
- Manual testing instructions: All non-trivial pull requests are expected to have a set of manual testing instructions with them. The more information you can provide to the reviewer(s) about how to verify the change, the more likely it is to get prioritized and the more likely you'll get detailed feedback.
- Review means manual testing AND code review: It's too easy to just say "LGTM". Nearly every PR needs to be compiled and tested manually in the review process.
- PR contributor pulls own PR if they have privs to do so: We've found that it works best for a PR submitter to pull their own PR after reviews are done and tests pass. The submitter typically knows any outstanding issues that need to be taken care of, and knows the best time for the PR to go in. If the submitter doesn't have the required privileges to merge, someone from the team with privileges will do it in consultation with the submitter.
- Rebase major PRs: Significant PRs may need to be rebased near the end of their life so that the tests run with all current code (to minimize the potential of breaking master on pull)
- Tests for new features and bugs: The default expectation is that pull requests will have automated tests and documentation updates included.
- Build Process: All repositories are expected to use our build tools and provide standard make targets for items such as building containers and binaries.