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

Feature: add labels when locking #4

Merged
merged 5 commits into from May 1, 2018

Conversation

3 participants
@hzoo
Copy link
Contributor

hzoo commented May 1, 2018

No description provided.

hzoo added some commits May 1, 2018

README.md Outdated
@@ -40,6 +40,8 @@ lockComment: >
# - no-locking
# Limit to only `issues` or `pulls`
# only: issues
# Add some labels when locking (useful when lockComment is false)
lockLabels: ['outdated']

This comment has been minimized.

@hzoo

hzoo May 1, 2018

Contributor

Not sure this needs to be # or not

This comment has been minimized.

@dessant

dessant May 1, 2018

Owner

Yes, let's comment it out.

src/lock.js Outdated
@@ -23,6 +23,16 @@ module.exports = class Lock {
body: lockComment
});
}

if (lockLabels) {

This comment has been minimized.

@hzoo

hzoo May 1, 2018

Contributor

Not sure if it should be plural or not. It accepts an array of strings so I did that even though personally I would just simply call it outdated. I did send a request for an outdated lock reason already so this might not be necessary when that happens later.

This comment has been minimized.

@dessant

dessant May 1, 2018

Owner

I think being able to set a single label might be enough, since the primary reason for it would be to easily list issues locked by the bot.

hzoo added some commits May 1, 2018

@hzoo

This comment has been minimized.

Copy link
Contributor

hzoo commented May 1, 2018

Ok made those changes

@dessant

This comment has been minimized.

Copy link
Owner

dessant commented May 1, 2018

Nice, thanks for taking the time! 😋

@dessant dessant merged commit 4c0bc99 into dessant:master May 1, 2018

@hzoo

This comment has been minimized.

Copy link
Contributor

hzoo commented May 1, 2018

Thanks for the responsiveness! haha

@dessant

This comment has been minimized.

Copy link
Owner

dessant commented May 1, 2018

Your features are live now.

@hzoo

This comment has been minimized.

Copy link
Contributor

hzoo commented May 1, 2018

@hzoo

This comment has been minimized.

Copy link
Contributor

hzoo commented May 3, 2018

Actually @dessant I was wondering if you knew why https://github.com/babel/babel/issues?q=label%3Aoutdated+is%3Aclosed only has 157 outdated labels/locked issues atm? I would of thought it was much faster to apply the lock?

It looks like it's supposed to do 30 issues an hour?

@Ben3eeE

This comment has been minimized.

Copy link

Ben3eeE commented May 3, 2018

@hzoo This is to prevent triggering abuse mechanisms on GitHub. See https://github.com/dessant/lock-threads#why-are-only-some-issues-and-pull-requests-locked

@dessant

This comment has been minimized.

Copy link
Owner

dessant commented May 3, 2018

Strange, because other projects continue to be locked. It may have something to do with the latest closed issues all being locked, and the app requesting only the first 30 from the list.

https://github.com/babel/babel/issues?q=updated%3A%3C2018-02-01+is%3Aclosed+sort%3Aupdated-desc

My guess is that we need to iterate through the results until we collect 30 lockable issues, I think there is no search filter for locked ones. I'll look at the exact api responses tomorrow and sort it out.

@dessant

This comment has been minimized.

Copy link
Owner

dessant commented May 3, 2018

Wait, this starts to look like an api regression, check the link in my previous comment, there are pull request threads listed that were updated recently, despite the updated:<2018-02-01 filter.

@Ben3eeE, am I missing something?

@dessant

This comment has been minimized.

Copy link
Owner

dessant commented May 3, 2018

@hzoo, it seems the filter interprets pull request updating as code changes, while the UI disagrees what updating a thread means:

screenshot from 2018-05-03 23-43-45

Issue update filtering is handled correctly, you could limit the bot to issues only and continue the locking process until this is sorted out.

https://github.com/babel/babel/issues?utf8=%E2%9C%93&q=updated%3A%3C2018-02-01+is%3Aclosed+sort%3Aupdated-desc+is%3Aissue

@hzoo

This comment has been minimized.

Copy link
Contributor

hzoo commented May 3, 2018

@Ben3eeE yeah I totally understand that and saw that note but I turned the bot 2 days ago so I would expect a lot more issues to be locked. It more feels like every time I turn on the bot or change a setting it would do the 30 issues instead of per hour. And yeah it only takes the latest instead of the oldest?

@dessant

This comment has been minimized.

Copy link
Owner

dessant commented May 3, 2018

@hzoo, sorting by oldest first would mean all the previously locked issues get listed first after the initial issue backlog is processed by the bot. The iteration I suggested above is also not practical, because you end up processing the entire issue list, searching for lockable ones.

The current way seems to be the only practical one until there is a filter for locked issues. Issue labeling could be made mandatory to be used as a filter, but that's undesirable.

@hzoo

This comment has been minimized.

Copy link
Contributor

hzoo commented May 3, 2018

Ooh I see, the way I'm using it does add a label though - could we add a feature that uses that as the filter (if you provide the label)?

@dessant

This comment has been minimized.

Copy link
Owner

dessant commented May 3, 2018

We could, but that won't be reliable either as a solution for projects in general and could cause this deadlock the same way if say you start setting the label after the first few issues are locked. I'd much rather see the update time filter for pull request threads fixed for the github api. 😛

@Ben3eeE

This comment has been minimized.

Copy link

Ben3eeE commented May 4, 2018

@hzoo Oh I misread your question 😊

@dessant Yeah the way the updated filter works for pull requests seems odd.

@dessant

This comment has been minimized.

Copy link
Owner

dessant commented May 4, 2018

Let's track this at #5.

@hzoo

This comment has been minimized.

Copy link
Contributor

hzoo commented May 7, 2018

Made a post about this https://twitter.com/left_pad/status/993492191996674048 since a lot of people were mentioning the issues with old comments on twitter!

@dessant

This comment has been minimized.

Copy link
Owner

dessant commented May 7, 2018

@hzoo Oh wow! Thanks for spreading awareness, and for the shout out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment