Add support for adding & removing labels when no longer stale #468

benvillalobos · 2021-05-26T00:23:42Z

Fixes #460

Changes

Add support for:

labels-to-remove-when-unstale
labels-to-add-when-unstale

Context

In order to prevent scenarios like this and to reduce the amount of upkeep required for maintainers, allow the action to remove/add labels to prevent marking issues stale over and over again.

Example of an ideal scenario is this:

User submits issue
We respond and need author feedback, apply that label
Issue goes stale via automation
Author (or someone else with the same issue) responds suggesting it's still an issue
Automation should remove the needs-author-feedback label and add some author-responded or needs-dev-response label

This allows automation to ONLY run on needs-author-feedback issues, or be exempt on author-responded issues.

src/classes/issues-processor.ts

benvillalobos · 2021-05-26T00:24:31Z

src/classes/issues-processor.ts

+      `Adding labels marked as add-labels-when-updated-from-stale...`
+    );
+
+    this._statistics?.incrementAddedItemsLabel(issue);


Not sure if this is the statistic to increment

This is a statistic to reflect a call to GitHub to add (one or multiple - we don't care since it's an array) a label to an issue/PR, so it seems OK to me, but I will have a better insight once I reviewed it.

src/classes/issues-processor.ts

src/interfaces/issues-processor-options.ts

C0ZEN · 2021-05-26T16:59:54Z

src/classes/issues-processor.ts

@@ -399,6 +402,8 @@ export class IssuesProcessor {
          issue,
          staleLabel,


You need to update the readme as well

C0ZEN · 2021-05-26T17:01:54Z

src/classes/issues-processor.ts

@@ -571,6 +578,10 @@ export class IssuesProcessor {
    if (this._shouldRemoveStaleWhenUpdated(issue) && issueHasComments) {
      await this._removeStaleLabel(issue, staleLabel);

+      // Are there labels to remove or add when an issue is no longer stale?
+      await this._removeLabelsWhenUpdatedFromStale(issue, removeLabelsWhenUpdatedFromStale);


It's nice to defer the logic to reduce the complexity of this file.
One thing that could maybe be even better is to completely defer it to a dedicated class/service like milestones and assignees works.

C0ZEN · 2021-05-26T17:02:33Z

src/classes/issues-processor.ts

@@ -911,6 +922,48 @@ export class IssuesProcessor {
    return this.options.removeStaleWhenUpdated;
  }

+  private async _removeLabelsWhenUpdatedFromStale(
+    issue: Issue,
+    labels: Readonly<string>[]


Suggested change

labels: Readonly<string>[]

labels: ReadonlyArray<string>

src/classes/issues-processor.ts

benvillalobos · 2021-06-03T18:29:31Z

src/classes/issues-processor.ts

+    );
+
+    for (const label of labels.values()) {
+      await this._removeLabel(issue, label);


Unless I'm missing something, it doesn't look like there's a single method for that: https://docs.github.com/en/rest/reference/issues#labels

Indeed.
In that case, it would be nice I think to explicitly mention in the readme when describing the option that it will consume one operation per label removal, and it's to use carefully. WDYT?

src/classes/issues-processor.ts

src/main.ts

C0ZEN · 2021-05-26T17:09:54Z

src/classes/issues-processor.ts

+    }
+  }
+
+  private async _addLabelsWhenUpdatedFromStale(


Add tests, please?

See #468 (comment), I should have replied to this comment.

src/classes/issues-processor.ts

benvillalobos · 2021-06-03T21:37:27Z

src/classes/issues-processor.ts

+
+  private async _addLabelsWhenUpdatedFromStale(
+    issue: Issue,
+    addLabels: Readonly<string>[]


Not sure if we can modify the parameter this is passing into, but changing it to a ReadonlyArray gets type complaints: `The type 'readonly string[]' is 'readonly' and cannot be assigned to the mutable type 'string[]'.'

src/classes/issues-processor.ts

benvillalobos · 2021-06-03T21:02:44Z

I noticed there was no easy way to check if specific labels were added / removed from issues, so I kept the "make sure some label was added to an issue" test similar to the others, where it just checks that an issue had a label added to it when we unstale it.

It's harder to do that for unstale-issue-has-label-removed without redesigning some of the code, which is more than I'd like to chew. I could add a test that removes a label on unstale and check if the array that contains "issues that had labels removed from them" contains two issues (_removeLabel was called twice and therefore added the same item twice), but it's not ideal.

luketomlinson

Good work @benvillalobos! Let's get the README updated with the new options. I also left a comment about naming.

luketomlinson · 2021-06-07T14:44:18Z

src/interfaces/issues-processor-options.ts

@@ -47,4 +47,6 @@ export interface IIssuesProcessorOptions {
  exemptAllIssueAssignees: boolean | undefined;
  exemptAllPrAssignees: boolean | undefined;
  enableStatistics: boolean;
+  removeLabelsWhenUnstale: string;


I don't want to be too picky on naming, but I wonder if a name like labelsToRemoveWhenUnstale would read a little better and indicate that this option is a list of labels. To me removeLabelsWhenUnstale reads like it should be a boolean option. Not a huge deal either way, but curious what your thoughts are on that? @benvillalobos @C0ZEN

reads like it should be a boolean option

I agree, and that makes sense considering I followed the naming scheme from removeIssueStaleWhenUpdated 😄 will rename.

benvillalobos · 2021-06-07T19:00:04Z

Just rebased off of main. When running npm run all I get some diffs that look to be mostly auto formatting. Should I push them up as a single commit or leave these out?

C0ZEN · 2021-06-07T19:07:00Z

@benvillalobos you need to commit all those changes (probably due to a recent npm i which updated deps due to unpinned dependencies).

C0ZEN

Almost done mate!

README.md

__tests__/main.spec.ts

C0ZEN · 2021-06-07T19:14:16Z

src/classes/issues-processor.ts

@@ -944,6 +959,65 @@ export class IssuesProcessor {
    return this.options.removeStaleWhenUpdated;
  }

+  private async _removeLabelsWhenUnstale(
+    issue: Issue,
+    removeLabels: Readonly<string>[]


Suggested change

removeLabels: Readonly<string>[]

labels: Readonly<string>[]

C0ZEN · 2021-06-07T19:14:55Z

src/classes/issues-processor.ts

+
+  private async _addLabelsWhenUnstale(
+    issue: Issue,
+    addLabels: Readonly<string>[]


Suggested change

addLabels: Readonly<string>[]

labels: Readonly<string>[]

haven't been ignoring this suggestion. Trying to keep the var's name as labels would result in an error passing labels: labels when calling the API, presumably because the names were identical.

src/classes/issues-processor.ts

C0ZEN · 2021-06-07T19:16:36Z

src/classes/issues-processor.ts

+
+    this.addedLabelIssues.push(issue);
+
+    if (this.options.debugOnly) {


Since the merge of this one #463
This check can now be closer to the GitHub call.
Basically, you can move the if from here to just around "addLabels".
That way, we have a better debugger and we can test the operations per run.

C0ZEN

luketomlinson · 2021-06-07T21:00:39Z

@benvillalobos some conflicts FYI

…fy arguments to remove ambiguity in 'labels' var & parameter

Co-authored-by: Geoffrey Testelin <geoffrey.testelin@gmail.com>

benvillalobos · 2021-06-07T23:02:56Z

dist/index.js

@@ -299,14 +300,16 @@ class IssuesProcessor {
            else {
                this._logger.info(`${logger_service_1.LoggerService.yellow('Processing the batch of issues')} ${logger_service_1.LoggerService.cyan(`#${page}`)} ${logger_service_1.LoggerService.yellow('containing')} ${logger_service_1.LoggerService.cyan(issues.length)} ${logger_service_1.LoggerService.yellow(`issue${issues.length > 1 ? 's' : ''}...`)}`);
            }
+            const labelsToAddWhenUnstale = words_to_list_1.wordsToList(this.options.labelsToAddWhenUnstale);


Is it reasonable to add this var as a global and have it set once via the constructor? I'm not a fan of changing the args to processIssue to pass labels to add/remove when unstale. Not the most familiar with best practices in ts though.

luketomlinson

Great work @benvillalobos!

benvillalobos · 2021-06-08T17:08:18Z

Thanks @luketomlinson! And thanks for the help shepherding this along @C0ZEN !

jezdez · 2021-08-19T11:13:36Z

Seems like this wasn't enabled in the action.yml: #551

benvillalobos requested a review from a team as a code owner May 26, 2021 00:23

benvillalobos commented May 26, 2021

View reviewed changes

C0ZEN reviewed May 26, 2021

View reviewed changes

C0ZEN reviewed Jun 3, 2021

View reviewed changes

luketomlinson requested changes Jun 7, 2021

View reviewed changes

benvillalobos force-pushed the dev/add-remove-label-on-unstale branch from ca82ec2 to 6389ef2 Compare June 7, 2021 18:33

C0ZEN reviewed Jun 7, 2021

View reviewed changes

C0ZEN approved these changes Jun 7, 2021

View reviewed changes

benvillalobos and others added 13 commits June 7, 2021 15:03

Add support for adding & removing labels when no longer stale

13d9c69

Add remove/addLabelsWhenUpdatedFromStale to relevant spec files. Modi…

beab20d

…fy arguments to remove ambiguity in 'labels' var & parameter

Change parameters for clarity, let autoformat do its thing

ea783f0

PR feedback: More useful logging when removing labels

f60db78

Wrap client calls in try catches

9f59678

Use Unstale in variable names

6127f76

Don't run add label logic under debug

cccedbc

Add test for labels added to unstale issues

4863cf4

PR Feedback: logging

cd16e94

Update README

aad3212

Rename vars to labels-to-add/remove-when-unstale

01bb785

Apply doc suggestions from code review

939e67f

Co-authored-by: Geoffrey Testelin <geoffrey.testelin@gmail.com>

PR Feedback

272bf5f

benvillalobos force-pushed the dev/add-remove-label-on-unstale branch from 87fbc5e to 272bf5f Compare June 7, 2021 23:00

benvillalobos commented Jun 7, 2021

View reviewed changes

luketomlinson approved these changes Jun 8, 2021

View reviewed changes

luketomlinson merged commit b1da9e1 into actions:main Jun 8, 2021

This was referenced Jun 8, 2021

Include option to ignore PR activity #435

Closed

Allow to add or remove labels on stale #498

Closed

jezdez mentioned this pull request Aug 19, 2021

labels-to-add-when-unstale and labels-to-remove-when-unstale missing from action.yml #551

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for adding & removing labels when no longer stale #468

Add support for adding & removing labels when no longer stale #468

benvillalobos commented May 26, 2021 •

edited

benvillalobos May 26, 2021

C0ZEN May 26, 2021

C0ZEN May 26, 2021

C0ZEN May 26, 2021

C0ZEN May 26, 2021

This comment was marked as resolved.

benvillalobos Jun 3, 2021

C0ZEN Jun 3, 2021

C0ZEN May 26, 2021

benvillalobos Jun 3, 2021 •

edited

This comment was marked as resolved.

benvillalobos Jun 3, 2021

benvillalobos commented Jun 3, 2021

luketomlinson left a comment

luketomlinson Jun 7, 2021

benvillalobos Jun 7, 2021

benvillalobos commented Jun 7, 2021

C0ZEN commented Jun 7, 2021

C0ZEN left a comment

C0ZEN Jun 7, 2021

C0ZEN Jun 7, 2021

benvillalobos Jun 7, 2021

C0ZEN Jun 7, 2021 •

edited

C0ZEN left a comment

luketomlinson commented Jun 7, 2021

benvillalobos Jun 7, 2021

luketomlinson left a comment

benvillalobos commented Jun 8, 2021

jezdez commented Aug 19, 2021

		@@ -399,6 +402,8 @@ export class IssuesProcessor {
		issue,
		staleLabel,


		this.addedLabelIssues.push(issue);

		if (this.options.debugOnly) {

Add support for adding & removing labels when no longer stale #468

Add support for adding & removing labels when no longer stale #468

Conversation

benvillalobos commented May 26, 2021 • edited

Changes

Context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos Jun 3, 2021 • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

benvillalobos commented Jun 3, 2021

luketomlinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos commented Jun 7, 2021

C0ZEN commented Jun 7, 2021

C0ZEN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0ZEN Jun 7, 2021 • edited

Choose a reason for hiding this comment

C0ZEN left a comment

Choose a reason for hiding this comment

luketomlinson commented Jun 7, 2021

Choose a reason for hiding this comment

luketomlinson left a comment

Choose a reason for hiding this comment

benvillalobos commented Jun 8, 2021

jezdez commented Aug 19, 2021

benvillalobos commented May 26, 2021 •

edited

benvillalobos Jun 3, 2021 •

edited

C0ZEN Jun 7, 2021 •

edited