Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update how stale handles exempt items #874

Merged
merged 6 commits into from Dec 20, 2022
Merged

Conversation

johnsudol
Copy link
Contributor

@johnsudol johnsudol commented Dec 2, 2022

Changes

  • New option added remove-stale-from-exempt-items
  • Updated how stale interacts with exempt items
  • Clarified some of the existing documentation

Context

Originally stale bot would not process exempt items.

In #136, stale bot was updated to remove stale labels from items with exempt labels. This introduced a problem where if a user wanted to manually add a stale label to an item with an exempt label, the stale label would automatically be removed.

To resolve this, I've added a flag remove-stale-from-exempt-items which removes the stale label from an exempt item with recent updates. This comes at the cost of a couple of api calls, so by default this option is set to false.

 When the flag is disabled stale will continue to not process exempt items

TLDR: Stale bot removed stale labels from exempt items no matter what. The new option remove-stale-from-exempt-items allows stale bot to only remove the stale label from exempt issues when they've been recently updated

This PR reverts #136 and adds additional documentation and logging in order to clarify the behaviour of exempt items

Resolves #766

@johnsudol johnsudol requested a review from a team as a code owner December 2, 2022 14:46
@johnsudol johnsudol self-assigned this Dec 2, 2022
@johnsudol johnsudol marked this pull request as draft December 2, 2022 14:50
@johnsudol johnsudol marked this pull request as ready for review December 2, 2022 15:11
Copy link

@vanZeben vanZeben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some language updates that I feel are better, feel free to pick and choose.

Also, we've talked offline about this but just to put the comments in a PR. I personally don't think we should be supporting #136 further and should revert that change entirely.

There are 2 logical ways to infer what the purpose of this action is;

Personally I feel that the second inference is more appropriate to the spirit of this action. It also means that we don't have to write edge-cases to actively prevent users from labeling their issue's/pr's, which really leads to a confusing user experience (see #766).

This would mean reverting this PR (which is now adding an additional configuration option to handle edge cases), as well as #136 and being more clear on definitions. I'll leave it to discussion from the rest of the team

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@JoannaaKL
Copy link
Contributor

JoannaaKL commented Dec 6, 2022

So to paraphrase @vanZeben there are two options:

  1. Do not allow the issue/pr to be marked as stale, as in #136
  2. Do not process the issue/pr with the stale label at all.
    I am also personally in favour of the option number 2 - from my perspective it is more understandable and, as mentioned above, relieves us from supporting all weird edge cases. It'd be also great to explicitly describe it in the docs. (Because if now we're discussing what does extempt mean, imagine someone seeing the plugin for the first time). 😄
    So I'd revert Remove stale label when issue is tagged with exempt label after marking stale #136 too and add more clarification docs.

__tests__/main.spec.ts Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
dist/index.js Show resolved Hide resolved
src/classes/issues-processor.ts Outdated Show resolved Hide resolved
@johnsudol
Copy link
Contributor Author

I've updated the PR to be aligned with comments above.

Moving forward this will simplify how stale handles exempt items and will allow us to avoid edge cases such as the flag that was previously mentioned in this PR

Copy link

@vanZeben vanZeben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@johnsudol johnsudol merged commit eed91cb into main Dec 20, 2022
@johnsudol johnsudol deleted the johnsudol/skip-exempt-issues branch December 20, 2022 20:35
@johnsudol johnsudol mentioned this pull request Dec 20, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stale label gets removed when manually added
4 participants