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

stale: don't stale issues labelled with "Type: bug" #12192

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Sep 10, 2019

Contribution description

This PR limit the stale bot to PRs only instead of also apply to issues. Most of the issues that were closed are still relevant (and should be fixed btw).

Testing procedure

Well, merge and see ?

Issues/PRs references

None

@aabadie aabadie requested a review from miri64 September 10, 2019 14:42
@aabadie aabadie added Area: tools Area: Supplementary tools Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Sep 10, 2019
@miri64
Copy link
Member

miri64 commented Sep 10, 2019

Could we maybe add "Type: question" issues to that?

@kaspar030
Copy link
Contributor

Most of the issues that were closed are still relevant (and should be fixed btw).

Still. This forces us to take another look. Six months of inactivity. A warning. Then it gets closed, with another hint.

IMO once the initial storm has passed, this will not happen in bulk, but maybe weekly. I think it is nice to be reminded.

(The question "What is an issue?" is more philosophical.)

@aabadie
Copy link
Contributor Author

aabadie commented Sep 10, 2019

This forces us to take another look

Indeed, but it can also be ignored and the still relevant issue is forgotten. Not every maintainer has subscribed to all notifications, so most of them may certainly have missed issues that could interest them.
For PR, this is a different issue, we want to avoid contributions from being staled because of inactivity: no update after review or pending review or no consensus.

@kaspar030
Copy link
Contributor

it can also be ignored and the still relevant issue is forgotten.

I'd like to argue that a relevant issue cannot be forgotten...

@jcarrano
Copy link
Contributor

This morning I was thinking "what the f? bugs should not be closed!", but later I realized it is OK and it us up to us to either bump the issue/pr to un-stale it, or place a "don't stale" label.

It will happen that people come with "bugs" that get no attention or that nobody can reproduce, and it is OK to let them stale.

@aabadie
Copy link
Contributor Author

aabadie commented Sep 10, 2019

I'd like to argue that a relevant issue cannot be forgotten

A 2 minutes search in the list of today's closed issues raised some relevant ones: #4371, #5106, #5421, #5031, #7208, #4798, #5769, #6681, #7575, #7365, #7220. And there are more.

Some of them are marked as bugs or questions (unanswered). Some are feature requests, does it mean that we won't implement them ?

Even if relevant is a rather subjective classification, I think issues shouldn't be marked as staled and later closed. Closing them because they are staled just means we put the dust under the carpet.

I'm not able to treat several hundreds of notifications email, in various fields, in a day of (busy) work. They will just be forgotten.

@miri64
Copy link
Member

miri64 commented Sep 10, 2019

I think today is special, as it is exactly one month ago that stale-bot was activated. I agree with @kaspar030 that instances like this will be more spread out in the future, so we should not panic now about the workload of today, as it is just the work we have left behind for month, years even.

Closing them because they are staled just means we put the dust under the carpet.

And keeping them open with nobody addressing them ever isn't? I quoted the argument by the stale-bot themselves already on maintainers when we were discussing the introduction, but I will link it again, as it is relevant to this discussion: https://github.com/probot/stale#is-closing-stale-issues-really-a-good-idea

They will just be forgotten.

The fact that they are closed is only an indicator that they were already forgotten about IMHO.

@miri64
Copy link
Member

miri64 commented Sep 10, 2019

(and our timeframes are way longer than the default config. If nothing happens in an issue for 6 month it gets staled. If nothing happens after that (no setting labels, no comment, nothing) then it gets closed, which IMHO then is only justified) Lastly, if an issue really shouldn't be staled for whatever reason, there is always State: don't stale.

@miri64
Copy link
Member

miri64 commented Sep 10, 2019

Lastly, if an issue really shouldn't be staled for whatever reason, there is always State: don't stale.

And yes, that requires also work, but that is a feature, not a bug ;-)

@aabadie
Copy link
Contributor Author

aabadie commented Sep 10, 2019

instances like this will be more spread out in the future

Indeed, but the bootstrap is bit abrupt. We are kind of loosing some reference issues, even if they were opened long ago .
For PRs, this is different since it requires in mean more work from the contributor and the maintainer to get them merged. For issue, the workload required to file a bug or to answer a "question" is way lower in general. It's just that most of us don't care of issues. And the actual situation reflects this: ~200 issues closed, ~100 PRs closed.

Hiding problems is very different from fixing them.

@kaspar030
Copy link
Contributor

It's just that most of us don't care of issues.

I don't agree with that statement. 😉

If you want to keep the stance that every issue opened has to be fully resolved or declared non-existant, well, then we cannot automatically close issues.
IMO that is not practical. Issues that noone saves from staleing after two warnings and 7 months of inactivity are by definition no pressing issues.
Issues that are important jump into your face, they cannot get forgotten.

Just looking at the last three issues:

#10937: feature request that noone seems to be willing to work on.
#9631: "not so nice" (slight inefficiency, no real problem) that noone seems to be willing to work on
#208: feature request that noone seems to be willing to work on

What do we gain by keeping issues like #9631, "X can be optimized"?
Are people gonna be bored and look through these?

What do we gain by keeping issues like #10937 #208 (feature requests)?
Do people get bored and look through what features were requested and start implementing them?

Staleing those is totally alright in my opinion.

Hiding problems is very different from fixing them.

Cannot argue with that, but we have to include the definition of "problem" into this discussion.

@aabadie
Copy link
Contributor Author

aabadie commented Sep 13, 2019

#10937: feature request that noone seems to be willing to work on

This is not an issue but a PR

@aabadie
Copy link
Contributor Author

aabadie commented Sep 13, 2019

#9631: "not so nice" (slight inefficiency, no real problem) that noone seems to be willing to work on

See #9631 (comment)

@kaspar030
Copy link
Contributor

kaspar030 commented Sep 13, 2019

May I propose as an alternative to make double-checking automatically closed issues a part of the release specs testing?

@kaspar030
Copy link
Contributor

See #9631 (comment)

True, but it was re-opened.

@miri64
Copy link
Member

miri64 commented Sep 13, 2019

I agree with @aabadie in one point only: We have a list of "Known issues" in our release notes, that should not be closed (they get attention at least by the release manager every 3 month anyway). How about a label, say Process: release: known issue for those. This would

  1. allow the stale bot to be configured to ignore them
  2. simplify automated release-note generation (something @aabadie and @MrKevinWeiss tried to do in the past)

What do you think about this as a compromise? The release manager just would need to look at all the issues after the last one marked with such a label, so there is another time benefit there, when it comes to marking those issues.

I'm cc'ing @kb2ma as he is the current release manager

@aabadie
Copy link
Contributor Author

aabadie commented Sep 13, 2019

What do you think about this as a compromise?

Seems like a good trade-off. But now what do we do with the 200 issues closed this week (which contains some relevant ones) ? Who is willing to look into them, decide which one is relevant (very subjective task) and eventually re-open ?

@miri64
Copy link
Member

miri64 commented Sep 13, 2019

Who is willing to look into them, decide which one is relevant (very subjective task) and eventually re-open ?

Without wanting to put to much load on the release manager, but that would be their task (or at least to delegate it). As of now they would have needed to go through the list of Known Issues anyway to look and see if they are still an issue.

@kb2ma
Copy link
Member

kb2ma commented Sep 18, 2019

Sorry for the tangent, but let me make sure how the "Known Issues" are generated for a release.

  1. Review the list of known issues from previous release
  2. Remove any issues that have been resolved
  3. Review the issues generated since the last release. Determine if any of them should be included in known issues, and add those that qualify to the list.

As it stands now, I need to add a new step:

2a. Un-stale any staled issues in the list of known issues that have not been fixed.

Also, the proposal here is to add a new label, Process: release: known issue and tag all of the existing known issues. This will mean that we won't need to do step 2a. in the future.

About these 200 issues. As we said, some of them may be already known issues and must be un-staled. Are you asking me to re-evaluate the rest of these issues to determine if they should be added to the list? Haven't we considered these issues in the past and apparently no one thought them significant enough to add to the list of known issues?

So, I guess the nub here is to know how an issue is qualified as a "known issue". I would appreciate any heuristics.

@miri64
Copy link
Member

miri64 commented Sep 18, 2019

Sorry for the tangent, but let me make sure how the "Known Issues" are generated for a release.

Given that I reply a bit longer as well below should we maybe split this out in a separate issue or mailing list thread?

  1. Review the list of known issues from previous release
  2. Remove any issues that have been resolved
  3. Review the issues generated since the last release. Determine if any of them should be included in known issues, and add those that qualify to the list.

As it stands now, I need to add a new step:

2a. Un-stale any staled issues in the list of known issues that have not been fixed.

Also, the proposal here is to add a new label, Process: release: known issue and tag all of the existing known issues. This will mean that we won't need to do step 2a. in the future.

Actually, 1-2 could completely be automated with the Process: release: known issue label. The basis for this is already exists with https://github.com/RIOT-OS/RIOT-release-manager/pull/9 (can you see that repo btw?). And yes, the new step 2a would be not necessary anymore.

About these 200 issues. As we said, some of them may be already known issues and must be un-staled. Are you asking me to re-evaluate the rest of these issues to determine if they should be added to the list? Haven't we considered these issues in the past and apparently no one thought them significant enough to add to the list of known issues?

So, I guess the nub here is to know how an issue is qualified as a "known issue". I would appreciate any heuristics.

I guess the heuristic is "important enough to mention it in the release notes", but I agree that this is somewhat tautological. I think a proper definition wasn't stated up to this point. The original arguments @OlegHahm stated to me were (from memory) "A big enough issue, that might be of interest to people and should be mentioned" (a "Known issue") vs. "Overcrowding the release notes with tiny bugs that, yes, should be fixed, but don't ring any alarm bells" (so those don't go into the "Known issues" section).

@kb2ma
Copy link
Member

kb2ma commented Sep 18, 2019

I think we're close to a resolution, so let's stay here. I like the heuristic for a known issue, and added it to the Managing a Release page. Thanks!

I actually started to look at the staled issues to see if I could make a reasonable judgement. The first interesting one I came across was #7116, and what do you know, it was a Known Issue for the 2019.07 release. So, I am OK with reviewing this list of staled issues.

I agree that Process: release: known issue label is a good way to flag issues we want to remember, and it will reduce effort at release time. So here are the to-dos:

  • I am happy to create this label and tag all of the known issues with it, including those that got staled.
  • If I am confident that another staled issue is a known issue, I will tag it. If I am not sure, I will summarize my thinking to a wiki page under the release, and put a message on the Maintainers list about the page -- asking for people knowlegeable in the affected area to either directly label the issue, or add a comment to the issue for discussion.
  • @miri64, will you update the Stale bot to not Stale issues with the label Process: release: known issue label?
  • I will update the Managing the Release page with the use of the label, and post a message on the Maintainers list. If there is heavy resistance, then worst case we delete the label. :-)

If this all sounds good, then I think we can close this issue.

@miri64
Copy link
Member

miri64 commented Sep 18, 2019

  • @miri64, will you update the Stale bot to not Stale issues with the label Process: release: known issue label?

Can do. But first the label would be required.

@miri64
Copy link
Member

miri64 commented Sep 18, 2019

I actually started to look at the staled issues to see if I could make a reasonable judgement. The first interesting one I came across was #7116, and what do you know, it was a Known Issue for the 2019.07 release. So, I am OK with reviewing this list of staled issues.

#7116 isn't staled, so I'm confused.

@kb2ma
Copy link
Member

kb2ma commented Sep 18, 2019

Once there is agreement, I'll create the label, then you can update the stale bot.

Typo on the issue -- I meant #7114, which actually was the progenitor of PR 7116.

@miri64
Copy link
Member

miri64 commented Sep 18, 2019

Typo on the issue -- I meant #7114, which actually was the progenitor of PR 7116.

Yes if it isn't fixed it should still be mentioned and kept open.

@aabadie
Copy link
Contributor Author

aabadie commented Sep 21, 2019

Just in case, I went through all issues automatically closed by the stale bot and marked as Type: Bug and reopened them when I thought it was necessary, e.g. when no one recently added a conclusion comment or when they weren't closed by the bot.

Also I don't think we need to add another known issues label. Skipping issues marked as Bug is already enough. If an issue is marked as a bug, it cannot be closed automatically, it should be fixed or at least a good reason for not fixing it should be given.

@kb2ma
Copy link
Member

kb2ma commented Sep 21, 2019

I was just starting to review the list myself, so thanks for that effort @aabadie. I don't have a strong opinion on the way forward.

I am OK as release manager to start with the list of known issues from 2019.07, remove those that have been closed, and add new ones since the last release that seem worthy of the list.

@aabadie
Copy link
Contributor Author

aabadie commented Sep 21, 2019

@kb2ma regarding how the list of known issues (and fixed issues) is generated in the changelog of a release, you can have a look at https://github.com/RIOT-OS/RIOT-release-manager/pull/9 (for non maintainers, sorry private repo). The idea is basically to look at issues labelled Type: Bug only to generate the list and after sub-filter the list with some Area: xxx label to sort them.

@aabadie aabadie force-pushed the pr/ci/stale_issues_only branch from d1657c9 to 3dda969 Compare September 21, 2019 10:05
@aabadie aabadie changed the title stale: only apply to PRs stale: don't stale issues labelled with "Type: bug" Sep 21, 2019
@aabadie
Copy link
Contributor Author

aabadie commented Sep 21, 2019

Updated this PR: now the idea is to skip issues labelled with "Type: bug".

@kb2ma
Copy link
Member

kb2ma commented Sep 21, 2019

Thanks for the link to the PR to automatically generate the list of known issues. I updated that section of the Managing a Release page so I don't forget about it.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

I'm fine with merging the concept of "Known issue" and "bug". Small bugs should be fixable quickly (i.e. during the release testing phase) anyway ;-). However, one minor nit.

.github/stale.yml Show resolved Hide resolved
@miri64
Copy link
Member

miri64 commented Sep 23, 2019

I still have some stomach ache however. This might lead to bugs still being ignored.

@miri64
Copy link
Member

miri64 commented Sep 25, 2019

@kaspar030 @kb2ma what do you say to this compromise?

@miri64
Copy link
Member

miri64 commented Sep 25, 2019

BTW: one more thing to consider: this will also make PRs that are marked as bug fixes not go stale anymore.

@kb2ma
Copy link
Member

kb2ma commented Sep 25, 2019

I agree that it's simpler to reuse the existing bug label to avoid the stale bot than to introduce a new label. Logically it is sensible to say, "We don't stale bugs."

@aabadie aabadie added Area: CI Area: Continuous Integration of RIOT components and removed Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Oct 1, 2019
@aabadie
Copy link
Contributor Author

aabadie commented Oct 1, 2019

@kb2ma @miri64 are you ok to squash this one ?

@miri64
Copy link
Member

miri64 commented Oct 1, 2019

Yes please!

@aabadie aabadie force-pushed the pr/ci/stale_issues_only branch from 1d59038 to 3a3c0d3 Compare October 1, 2019 10:05
@aabadie
Copy link
Contributor Author

aabadie commented Oct 1, 2019

Done!

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK. Though this will mean bug fixes might stale, bug reports are not closed automatically anymore. Follow-up suggestion: Maybe we should split bug / bugfix as categories?

@aabadie
Copy link
Contributor Author

aabadie commented Oct 1, 2019

Though this will mean bug fixes might stale, bug reports are not closed automatically anymore.

I don't see this as a problem. As I already said, if an issue is marked as a bug it should be fixed or a good reason given to not fix it.
The same applies for PRs marked as bug: either we merge because the fix is valid, or we close it.

Having staled bugs (fixes or reports) should not happen, the community should handle them in priority, especially during the feature freeze period, before a release.

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 1, 2019
@aabadie aabadie merged commit 6b96f69 into RIOT-OS:master Oct 1, 2019
@aabadie aabadie deleted the pr/ci/stale_issues_only branch October 1, 2019 15:30
@kb2ma kb2ma added this to the Release 2019.10 milestone Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CI Area: Continuous Integration of RIOT components Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants