Separate code reviews into feedback levels
When providing feedback in a code review, it is useful to classify the feedback in three levels:
-
blocker: The change should not be integrated due to severe defects, such as data loss and interruption of service. A change with too-low test coverage is also a good reason for a blocker, as is a change that would incur too much tech debt.
-
recommendation: The change contains a problem that should not block integration, but would best be addressed soon after. Recommendations are typically refactorings.
-
nitpick: The change contains a small problem (based on the reviewer’s opinion) that should not block integration, and might not be worth addressing.
This is still relevant during outages, though the definition of a “blocker” could be relaxed, e.g. by removing the test coverage requirement.
Concerns
-
Ignoring non-blocking comments: I have on multiple occasions seen “nitpick” and “recommendation” comments on code reviews be ignored, forever. The unwritten assumption seems to be that anything that isn’t a blocker does not need to be addressed.
It could be the case that explicit feedback levels facilitate indicating which pieces of feedback is ignorable. Not having explicit feedback levels might even be beneficial, as it forces the author to handle each comment based on its content, rather than its feedback level.
-
Forcing all comments to be resolved before merging: GitLab has a configuration option (per-repository, I believe) that requires all comments to be marked as resolved before a changeset (MR) can be merged. This leads to comments being marked as resolved even if they are still relevant.
GitLab supports converting comments into issues, but from my experiences those issues linger and are never addressed.