Skip to content

Commit

Permalink
BC: Do not run stale logic for newly stale objects
Browse files Browse the repository at this point in the history 
https://github.com/jsoref/stale-bot-debug/actions/runs/5828506506/job/15806335430#step:2:97

::group::[actions#2] Issue actions#2
[actions#2] Issue actions#2
  [actions#2] Found this issue last updated at: 2023-08-09T14:33:12Z
...
  [actions#2] Marking this issue as stale
  [actions#2] This issue is now stale
  [actions#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...

  [actions#2] Checking for label on this issue
  [actions#2] Issue marked stale on: 2023-08-11T03:11:21Z
  [actions#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.

  [actions#2] Comments that are not the stale comment or another bot: 0
  [actions#2] Issue has been commented on: false
  [actions#2] Days before issue close: 3
  [actions#2] The option remove-stale-when-updated (​https://github.com/actions/stale#remove-stale-when-updated​) is: true
  [actions#2] The stale label should not be removed
  [actions#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:

  [actions#2] Removing all the labels specified via the labels-to-remove-when-stale (​https://github.com/actions/stale#labels-to-remove-when-stale​) option.
  [actions#2] Removing the label "label-to-add-when-unstale" from this issue...
  Error: [actions#2] Error when removing the label: "Label does not exist"

We return to nonsensical tasks:

  [actions#2] Issue has been updated since it was marked stale: false
  [actions#2] Issue has been updated in the last 3 days: true
  [actions#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.

  [actions#2] 5 operations consumed for this issue
  ::endgroup::
  • Loading branch information
@jsoref
jsoref committed Aug 11, 2023
1 parent 184e7af commit bf388bf
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 32 deletions.
17 changes: 5 additions & 12 deletions dist/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)})`);
Expand All @@ -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);
});
Expand Down Expand Up @@ -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;
Expand All @@ -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?
Expand Down
2 changes: 0 additions & 2 deletions src/classes/issue.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 {
Expand Down
26 changes: 8 additions & 18 deletions src/classes/issues-processor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -518,18 +523,14 @@ 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,
staleLabel,
staleMessage,
labelsToAddWhenUnstale,
labelsToRemoveWhenUnstale,
labelsToRemoveWhenStale,
closeMessage,
closeLabel
);
Expand Down Expand Up @@ -655,7 +656,6 @@ export class IssuesProcessor {
staleMessage: string,
labelsToAddWhenUnstale: Readonly<string>[],
labelsToRemoveWhenUnstale: Readonly<string>[],
labelsToRemoveWhenStale: Readonly<string>[],
closeMessage?: string,
closeLabel?: string
) {
Expand Down Expand Up @@ -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(
Expand All @@ -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`
Expand Down

0 comments on commit bf388bf

Please sign in to comment.