-
Notifications
You must be signed in to change notification settings - Fork 10.6k
How to create a successful pull request
Pull requests in rocket.chat are automatically merged as soon as they meet certain requirements:
- At least one approver (and from all codeowners)
- No comments pending resolution
- All green ci tasks
- Label marked as 'stat: QA skipped' or 'stat: QA tested'
At rocket.chat we have several automations that notify teams when there is a new review pending.
If you open a pull request "read to review" the team will be notified. The team will look at your PR with CI issues, description issues, or even missing changes, they will then ignore it because it looks like an incomplete work and your PR lost its momentum, and went to a potential forgotten queue.
So the suggestion on how to deal is Create pull request in draft do everything you need to do Create tests for the proposed problem Ask a colleague, check if it looks minimally good Ask a colleague to test if the solution does what it needs Adjust the PR title Make sure the description is good Put all related issues in the description Ensure all CI tests and checks are passing
Then, only now, finally, you should move your pr from Draft
to Ready to review
This is a rule that doesn't apply to community members. As soon as you start working on a task you know when it should be released. Whether it is a correction, an improvement or a chore. So to ensure that your precious work will be released at the right time, put the milestone. This helps when making the release, enabling the releasers to monitor PRS that are still pending.
Here at rocket.chat we decided to make a changelog focused on the person who manages rocket.chat, not necessarily the developer.
So we created some rules (similar to conventional commits) to help organize the changelog generation
-
Regression: Your title here...
The term regression is used a little differently here on rocket.chat Regression means a bug introduced in the development period, which is being fixed before being released. No one has ever seen this problem in production, as it never existed in production. This is a regression. This pull request and its description will be omitted from the release. -
Chore: Your title here
Things that don't matter to the customer/end user. Like: CI tweaks, rewrites that don't change behavior, Testing -
[NEW] Your title here...
This one applies to new stuff, which will be released in the future, NEVER expect a new one to make it into previous releases -
[IMPROVE] Your title here...
Similar toNEW
, but it is about improvements in the features that already exist -
[BREAK] Your title here...
This is not something you see every day, but it needs to be remembered. When we decide to change some functionality, or remove some functionality, it can affect the customer's life and he needs to know. Be careful, as this PR will be waiting for a MAJOR release, this may take some time.
It's very likely that when you make a change you also test the effects of that change, and that's great! However, it is a good practice that we create automated tests for those changes. So if you want to avoid extra work and review requests, create automated tests (e2e and unit).
This is a rule may not apply to community members. You are responsible for your PR. Request changes? questions? conflicts? deadlines? and even reviews! you are responsible for managing all of those. Remember that in a company other developers are doing other task, no one else should be more concerned about this delivery other than you, so create a routine and keep your PRs always sharp. Another thing is deadlines, don't expect your pull request with 10000 files changes to be reviewed at the last minute, as soon as you mark it as done, ensure a comfortable deadline so that others can do their work correctly (QA+review).