[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:
Backports
Related
related to SERVER-56216 Commit queue should verify no open TO... Closed
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 SERVER-48291, but it isn't a resolution of SERVER-48291, so it is incorrect to remove the TODOs. However, I can't get a green patch build or send it through the commit queue since it will be rejected by the TODO checker. A mechanism to bypass this check for instances where its application is inappropriate should be provided. Not every commit is a resolution, and not every TODO is made stale on commits mentioning a ticket.



 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 ]

does the commit queue have the option to give a warning rather than blocking merge? If so, would that work as a compromise?

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.

Generated at Thu Feb 08 05:42:46 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.