From bf388bf594a023da9f3bd70afc17d2671ff94dda Mon Sep 17 00:00:00 2001 From: Josh Soref <2119212+jsoref@users.noreply.github.com> Date: Fri, 11 Aug 2023 09:06:11 -0400 Subject: [PATCH] BC: Do not run stale logic for newly stale objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/jsoref/stale-bot-debug/actions/runs/5828506506/job/15806335430#step:2:97 ::group::[#2] Issue #2 [#2] Issue #2 [#2] Found this issue last updated at: 2023-08-09T14:33:12Z ... [#2] Marking this issue as stale [#2] This issue is now stale [#2] This issue is already stale At this point, things are already pretty bad. The issue wasn't _already_ stale, it was _just marked_ stale. There was a lot of code trying to keep this state in mind, and it yields some really lousy outcomes... [#2] Checking for label on this issue [#2] Issue marked stale on: 2023-08-11T03:11:21Z [#2] Checking for comments on issue since: 2023-08-11T03:11:21Z Great, so, while the workflow is running, it's looking to see if any new comments have arrived since the workflow itself marked the issue as stale. The odds of there being any are very close to 0, and really there's no point in checking now, it makes much more sense to check the _next_ time the workflow runs. [#2] Comments that are not the stale comment or another bot: 0 [#2] Issue has been commented on: false [#2] Days before issue close: 3 [#2] The option remove-stale-when-updated (​https://github.com/actions/stale#remove-stale-when-updated​) is: true [#2] The stale label should not be removed [#2] marked stale this run, so don't check for updates Here we finally think about the fact that we're in this edge case. The only thing that makes sense to keep, and which is moved by this change is: [#2] Removing all the labels specified via the labels-to-remove-when-stale (​https://github.com/actions/stale#labels-to-remove-when-stale​) option. [#2] Removing the label "label-to-add-when-unstale" from this issue... Error: [#2] Error when removing the label: "Label does not exist" We return to nonsensical tasks: [#2] Issue has been updated since it was marked stale: false [#2] Issue has been updated in the last 3 days: true [#2] Stale issue is not old enough to close yet (hasComments? false, hasUpdate? true) Note that it's a really rude behavior to mark an item as stale and close it in the same action. BEHAVIOR CHANGE: This change will force action users to trigger two workflow runs if they want to retain that rude behavior. [#2] 5 operations consumed for this issue ::endgroup:: --- dist/index.js | 17 +++++------------ src/classes/issue.ts | 2 -- src/classes/issues-processor.ts | 26 ++++++++------------------ 3 files changed, 13 insertions(+), 32 deletions(-) diff --git a/dist/index.js b/dist/index.js index 343c8313a..8277d830e 100644 --- a/dist/index.js +++ b/dist/index.js @@ -288,7 +288,6 @@ class Issue { this.milestone = issue.milestone; this.assignees = issue.assignees || []; this.isStale = (0, is_labeled_1.isLabeled)(this, this.staleLabel); - this.markedStaleThisRun = false; } get isPullRequest() { return (0, is_pull_request_1.isPullRequest)(this); @@ -626,8 +625,8 @@ class IssuesProcessor { issueLogger.info(`This $$type should be marked as stale based on the option ${issueLogger.createOptionLink(this._getDaysBeforeStaleUsedOptionName(issue))} (${logger_service_1.LoggerService.cyan(daysBeforeStale)})`); yield this._markStale(issue, staleMessage, staleLabel, skipMessage); issue.isStale = true; // This issue is now considered stale - issue.markedStaleThisRun = true; issueLogger.info(`This $$type is now stale`); + yield this._removeLabelsOnStatusTransition(issue, labelsToRemoveWhenStale, option_1.Option.LabelsToRemoveWhenStale); } else { issueLogger.info(`This $$type should not be marked as stale based on the option ${issueLogger.createOptionLink(this._getDaysBeforeStaleUsedOptionName(issue))} (${logger_service_1.LoggerService.cyan(daysBeforeStale)})`); @@ -642,10 +641,9 @@ class IssuesProcessor { } } } - // Process the issue if it was marked stale - if (issue.isStale) { + else { issueLogger.info(`This $$type is already stale`); - yield this._processStaleIssue(issue, staleLabel, staleMessage, labelsToAddWhenUnstale, labelsToRemoveWhenUnstale, labelsToRemoveWhenStale, closeMessage, closeLabel); + yield this._processStaleIssue(issue, staleLabel, staleMessage, labelsToAddWhenUnstale, labelsToRemoveWhenUnstale, closeMessage, closeLabel); } IssuesProcessor._endIssueProcessing(issue); }); @@ -752,7 +750,7 @@ class IssuesProcessor { }); } // handle all of the stale issue logic when we find a stale issue - _processStaleIssue(issue, staleLabel, staleMessage, labelsToAddWhenUnstale, labelsToRemoveWhenUnstale, labelsToRemoveWhenStale, closeMessage, closeLabel) { + _processStaleIssue(issue, staleLabel, staleMessage, labelsToAddWhenUnstale, labelsToRemoveWhenUnstale, closeMessage, closeLabel) { return __awaiter(this, void 0, void 0, function* () { const issueLogger = new issue_logger_1.IssueLogger(issue); const markedStaleOn = (yield this.getLabelCreationDate(issue, staleLabel)) || issue.updated_at; @@ -771,18 +769,13 @@ class IssuesProcessor { else { issueLogger.info(`The stale label should be removed if all conditions met`); } - if (issue.markedStaleThisRun) { - issueLogger.info(`marked stale this run, so don't check for updates`); - yield this._removeLabelsOnStatusTransition(issue, labelsToRemoveWhenStale, option_1.Option.LabelsToRemoveWhenStale); - } // The issue.updated_at and markedStaleOn are not always exactly in sync (they can be off by a second or 2) // isDateMoreRecentThan makes sure they are not the same date within a certain tolerance (15 seconds in this case) const issueHasUpdateSinceStale = (0, is_date_more_recent_than_1.isDateMoreRecentThan)(new Date(issue.updated_at), new Date(markedStaleOn), 15); issueLogger.info(`$$type has been updated since it was marked stale: ${logger_service_1.LoggerService.cyan(issueHasUpdateSinceStale)}`); // Should we un-stale this issue? if (shouldRemoveStaleWhenUpdated && - (issueHasUpdateSinceStale || issueHasCommentsSinceStale) && - !issue.markedStaleThisRun) { + (issueHasUpdateSinceStale || issueHasCommentsSinceStale)) { issueLogger.info(`Remove the stale label since the $$type has been updated and the workflow should remove the stale label when updated`); yield this._removeStaleLabel(issue, staleLabel); // Are there labels to remove or add when an issue is no longer stale? diff --git a/src/classes/issue.ts b/src/classes/issue.ts index b90631835..e80ae9527 100644 --- a/src/classes/issue.ts +++ b/src/classes/issue.ts @@ -21,7 +21,6 @@ export class Issue implements IIssue { readonly milestone?: IMilestone | null; readonly assignees: Assignee[]; isStale: boolean; - markedStaleThisRun: boolean; operations = new Operations(); private readonly _options: IIssuesProcessorOptions; @@ -42,7 +41,6 @@ export class Issue implements IIssue { this.milestone = issue.milestone; this.assignees = issue.assignees || []; this.isStale = isLabeled(this, this.staleLabel); - this.markedStaleThisRun = false; } get isPullRequest(): boolean { diff --git a/src/classes/issues-processor.ts b/src/classes/issues-processor.ts index 31bbb99d6..73aa14fc8 100644 --- a/src/classes/issues-processor.ts +++ b/src/classes/issues-processor.ts @@ -494,8 +494,13 @@ export class IssuesProcessor { ); await this._markStale(issue, staleMessage, staleLabel, skipMessage); issue.isStale = true; // This issue is now considered stale - issue.markedStaleThisRun = true; issueLogger.info(`This $$type is now stale`); + + await this._removeLabelsOnStatusTransition( + issue, + labelsToRemoveWhenStale, + Option.LabelsToRemoveWhenStale + ); } else { issueLogger.info( `This $$type should not be marked as stale based on the option ${issueLogger.createOptionLink( @@ -518,10 +523,7 @@ export class IssuesProcessor { ); } } - } - - // Process the issue if it was marked stale - if (issue.isStale) { + } else { issueLogger.info(`This $$type is already stale`); await this._processStaleIssue( issue, @@ -529,7 +531,6 @@ export class IssuesProcessor { staleMessage, labelsToAddWhenUnstale, labelsToRemoveWhenUnstale, - labelsToRemoveWhenStale, closeMessage, closeLabel ); @@ -655,7 +656,6 @@ export class IssuesProcessor { staleMessage: string, labelsToAddWhenUnstale: Readonly[], labelsToRemoveWhenUnstale: Readonly[], - labelsToRemoveWhenStale: Readonly[], closeMessage?: string, closeLabel?: string ) { @@ -702,15 +702,6 @@ export class IssuesProcessor { ); } - if (issue.markedStaleThisRun) { - issueLogger.info(`marked stale this run, so don't check for updates`); - await this._removeLabelsOnStatusTransition( - issue, - labelsToRemoveWhenStale, - Option.LabelsToRemoveWhenStale - ); - } - // The issue.updated_at and markedStaleOn are not always exactly in sync (they can be off by a second or 2) // isDateMoreRecentThan makes sure they are not the same date within a certain tolerance (15 seconds in this case) const issueHasUpdateSinceStale = isDateMoreRecentThan( @@ -728,8 +719,7 @@ export class IssuesProcessor { // Should we un-stale this issue? if ( shouldRemoveStaleWhenUpdated && - (issueHasUpdateSinceStale || issueHasCommentsSinceStale) && - !issue.markedStaleThisRun + (issueHasUpdateSinceStale || issueHasCommentsSinceStale) ) { issueLogger.info( `Remove the stale label since the $$type has been updated and the workflow should remove the stale label when updated`