[SERVER-57777] TODO checker needs to be able to be disabled for patch builds and commit queue merges Created: 17/Jun/21 Updated: 08/Jan/24 Resolved: 28/Jun/21 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 5.1.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Andrew Morrow (Inactive) | Assignee: | David Bradford (Inactive) |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||
| Operating System: | ALL | ||||||||||||
| Backport Requested: |
v5.0, v4.4
|
||||||||||||
| Sprint: | DAG 2021-07-12 | ||||||||||||
| Participants: | |||||||||||||
| Story Points: | 1 | ||||||||||||
| Description |
|
I have a commit I need to make under |
| Comments |
| Comment by Vivian Ge (Inactive) [ 06/Oct/21 ] |
|
Updating the fixversion since branching activities occurred yesterday. This ticket will be in rc0 when it’s been triggered. For more active release information, please keep an eye on #server-release. Thank you! |
| Comment by David Bradford (Inactive) [ 28/Jun/21 ] |
|
The todo check task has been removed from the commit-queue. |
| Comment by David Bradford (Inactive) [ 23/Jun/21 ] |
|
Ok, unless I hear any strong objections, I'm going to plan on removing the commit-queue task (while keeping the other checks). |
| Comment by Andrew Morrow (Inactive) [ 22/Jun/21 ] |
|
sara.williamson - Yes, that is correct. david.bradford - I think keeping the check in patch builds is great - if I really did forget to clean up the TODOs I'll get a red test. And, I can chose to ignore that red if I know I'm going to keep working on the ticket. But blocking merge in the commit queue feels like it goes too far. The commit queue is supposed to be the last check that the thing being submitted isn't wrong. But an overlooked TODO doesn't break the build or stop others from making forward progress. And preventing incremental work on a ticket seems like an undesired outcome to me, as does requiring making new tickets - just seems like busywork. |
| Comment by David Bradford (Inactive) [ 22/Jun/21 ] |
There isn't a good way to provide a warning in the commit queue. We have added a task that performs the same checks to required patch builds. It relies on engineers including the Jira ticket they are working on as part of the patch description, but if they do that they will see a failure in their patch build as a warning. |
| Comment by Ian Whalen (Inactive) [ 21/Jun/21 ] |
|
hey all, I am probably so far removed from the day-to-day of the server workflow that I shouldn't be involved here beyond offering insight into how the old TODO checker worked. sara.williamson is going to be best for handling or delegating any TPM involvement. |
| Comment by David Bradford (Inactive) [ 17/Jun/21 ] |
|
This discussion has come up before. So I want to ping the previous participates to get any of their thoughts. ian.whalen judah.schvimer louis.williams max.hirschhorn. From my perspective, I don't think adding support to bypass commit-queue checks is a good idea. That effectively defaults the whole purpose of having a commit-queue. So, I think the question becomes do we want to check for todo comments are part of the commit-queue or not. The argument for having it in the commit-queue is that you get fast feedback on whether there are any unresolved todos in the code you are trying to merge as opposed to getting a ticket on your backlog sometime after you've finished the ticket. The argument against having it is the commit-queue is that you can't leave todos against the commit you are working on in the code if you are breaking a ticket into multiple commits. |