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

feat(remove-stale-when-updated): Split remove-stale-when-updated option into remove-stale-when-updated and remove-stale-when-commented #390

Closed

Conversation

francoiscampbell
Copy link

@francoiscampbell francoiscampbell commented Mar 15, 2021

Currently, although updated PRs are not closed when updated, the stale label is not removed, even after re-running the action:
updated-pr-still-stale
(redacted because this is not my PR)

This PR also removes the label when the PR is updated (for example by pushing a new commit).

This PR splits the options to remove the stale by renaming remove-stale-when-updated to remove-stale-when-commented and introducing a new meaning to remove-stale-when-updated, which is to remove the stale label when the stale issue is updated in other ways than comments (typically pushing a new commit).

This also happens to fix (what I think is) a bug that was reported to me today: if an issue has a stale label and then updated without comments, the stale timer isn't reset and the issue is still closed after days-before-close, not days-before-stale + days-before-close as expected.

@francoiscampbell francoiscampbell marked this pull request as ready for review March 15, 2021 22:10
@C0ZEN
Copy link
Contributor

C0ZEN commented Mar 16, 2021

@francoiscampbell I saw this not long ago in the code #373 (comment)
I do not know if this is an issue or a choice but I think that at the end the action should have a new option to split which kind of trigger should remove the stale (comment or/and update).
And I would prefer to see a release containing this fix with the new option because if the bug is here since a long time people are used to it and basically changing the behaviour could result to unexpected un-stale.
What do you think?

@francoiscampbell
Copy link
Author

Oh sure. I had originally assumed (we just started using this action yesterday) that the label was an indication of whether the PR was "eligible" for closing or not, and felt that if updating the PR delays the closing, it's not stale anymore and therefore the label should be removed.

It's unfortunate that the current name of the option is remove-stale-when-updated, since its behaviour is more like remove-stale-when-commented, and remove-stale-when-updated is what I would've used for this PR's use-case. Any suggestions for the name?

@C0ZEN
Copy link
Contributor

C0ZEN commented Mar 16, 2021

I like the name of the options you propose. It makes sense and you can transform a bug into a fix with a new feature giving more control so I like that.

@francoiscampbell francoiscampbell marked this pull request as draft March 25, 2021 16:13
@francoiscampbell francoiscampbell changed the title feat(remove-stale-when-updated): remove stale label when pr is updated feat(remove-stale-when-updated): Split remote-stale-when-updated option into remote-stale-when-updated and remote-stale-when-commented Mar 25, 2021
@francoiscampbell francoiscampbell marked this pull request as ready for review March 25, 2021 16:34
@francoiscampbell
Copy link
Author

@C0ZEN Sounds good, I've gone and done the option split. Also updated the the PR title and description.

@C0ZEN
Copy link
Contributor

C0ZEN commented Mar 25, 2021

Can you add the option in the readme?

…on into remote-stale-when-updated and remote-stale-when-commented
@francoiscampbell
Copy link
Author

Ah oops, done.

@francoiscampbell
Copy link
Author

@C0ZEN Do you think this is ready to merge?

Copy link
Contributor

@C0ZEN C0ZEN left a comment

Choose a reason for hiding this comment

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

@francoiscampbell francoiscampbell changed the title feat(remove-stale-when-updated): Split remote-stale-when-updated option into remote-stale-when-updated and remote-stale-when-commented feat(remove-stale-when-updated): Split remove-stale-when-updated option into remove-stale-when-updated and remove-stale-when-commented Apr 15, 2021
@C0ZEN
Copy link
Contributor

C0ZEN commented Apr 28, 2021

@francoiscampbell conflicts FYI

@@ -32,7 +32,8 @@ Every argument is optional.
| `only-pr-labels` | Only PRs with ALL these labels are checked.<br>Separate multiple labels with commas (eg. "question,answered").<br>_Override `only-labels`_. |
| `any-of-labels` | Only issues and PRs with ANY of these labels are checked.<br>Separate multiple labels with commas (eg. "incomplete,waiting-feedback"). |
| `operations-per-run` | Maximum number of operations per run.<br>GitHub API CRUD related.<br>_Defaults to **30**_. |
| `remove-stale-when-updated` | Remove stale label from issue/PR on updates or comments.<br>_Defaults to **true**_. |
| `remove-stale-when-commented` | Remove stale label from issue/PR on comments.<br>_Defaults to **true**_. |
| `remove-stale-when-updated` | Remove stale label from issue/PR on any update, including comments.<br>_Defaults to **true**_. |
Copy link

Choose a reason for hiding this comment

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

Doesn't this make more sense? I can't tell if this is doc typo or intentional as I am not familiar with the code base.

Suggested change
| `remove-stale-when-updated` | Remove stale label from issue/PR on any update, including comments.<br>_Defaults to **true**_. |
| `remove-stale-when-updated` | Remove stale label from issue/PR on any update, excluding comments.<br>_Defaults to **true**_. |

@T6751
Copy link

T6751 commented Jun 3, 2021

Can i fix these conflicts and open PR?
@C0ZEN
@francoiscampbell

@github-actions github-actions bot removed the Stale label Jun 4, 2021
@C0ZEN
Copy link
Contributor

C0ZEN commented Jun 7, 2021

Since everyone is waiting for this I will do it.

C0ZEN added a commit to C0ZEN/stale that referenced this pull request Jun 7, 2021
C0ZEN added a commit to C0ZEN/stale that referenced this pull request Jun 7, 2021
Helping to close actions#441, actions#470, actions#435?
Closes actions#390 due to no activity

BREAKING CHANGES: the options related to remove-stale-when-updated will only check the updates, not the comment. It is only impactint the configurations using the value at false
C0ZEN added a commit to C0ZEN/stale that referenced this pull request Jun 8, 2021
Helping to close actions#441, actions#470, actions#435?
Closes actions#390 due to no activity

BREAKING CHANGES: the options related to remove-stale-when-updated will only check the updates, not the comment. It is only impactint the configurations using the value at false
C0ZEN added a commit to C0ZEN/stale that referenced this pull request Jun 10, 2021
Helping to close actions#441, actions#470, actions#435?
Closes actions#390 due to no activity

BREAKING CHANGES: the options related to remove-stale-when-updated will only check the updates, not the comment. It is only impactint the configurations using the value at false
luketomlinson pushed a commit that referenced this pull request Jun 14, 2021
* feat(options): add new options to avoid stale based on comments

Helping to close #441, #470, #435?
Closes #390 due to no activity

BREAKING CHANGES: the options related to remove-stale-when-updated will only check the updates, not the comment. It is only impactint the configurations using the value at false

* style(readme): fix table syntax due to rebase

* docs(readme): add permissions only for the new options
C0ZEN added a commit to C0ZEN/stale that referenced this pull request Jun 18, 2021
Helping to close actions#441, actions#470, actions#435?
Closes actions#390 due to no activity

BREAKING CHANGES: the options related to remove-stale-when-updated will only check the updates, not the comment. It is only impactint the configurations using the value at false
C0ZEN added a commit to C0ZEN/stale that referenced this pull request Jun 24, 2021
Helping to close actions#441, actions#470, actions#435?
Closes actions#390 due to no activity

BREAKING CHANGES: the options related to remove-stale-when-updated will only check the updates, not the comment. It is only impactint the configurations using the value at false
C0ZEN added a commit to C0ZEN/stale that referenced this pull request Jul 18, 2021
Helping to close actions#441, actions#470, actions#435?
Closes actions#390 due to no activity

BREAKING CHANGES: the options related to remove-stale-when-updated will only check the updates, not the comment. It is only impactint the configurations using the value at false
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.

4 participants