kubernetes / test-infra

Test infrastructure for the Kubernetes project.
Apache License 2.0
3.82k stars 2.64k forks source link

add support for `lifecycle/active` #8811

Closed neolit123 closed 6 years ago

neolit123 commented 6 years ago

we had a discussion today with @BenTheElder, @timothysc and @roberthbailey at the SIG Cluster Lifecycle meeting, about the fact that we have a custom label called active in the k/kubeadm repository.

the idea of the label is to indicate to repo maintainers and contributors that someone is working on the issue already. then it came to topic that issue "state" active seems like being part of the issue lifecycle itself and we already have these supported in prow: https://github.com/kubernetes/test-infra/blob/master/prow/plugins/lifecycle/lifecycle.go#L30

maybe active is really part of the lifecycle of the issue and it should be supported? then, if the developer stops working on the issue for some reason, lifecycle/active can go away in favor of lifecycle/stale

WDYT?

/cc @fejta /cc @kubernetes/sig-testing-feature-requests

edit: BTW, if this is approved i can help with parts of the code changes. :+1:

timothysc commented 6 years ago

+1 for bot usage and adding a state.

timothysc commented 6 years ago

/assign @BenTheElder

Only b/c he was there ;-)

BenTheElder commented 6 years ago

Happy to help implement, I think we might want to run this by @cblecker @spiffxp @kubernetes/sig-contributor-experience-feature-requests

/lifecycle active edit: yep, doesn't work as of now, but that's an easy change

stevekuznetsov commented 6 years ago

Is this a proposal to add this to all PRs until lifecycle/stale is needed? Or just to support the user command /lifecycle active?

spiffxp commented 6 years ago

IIRC, this is what the status/in-progress label is supposed to be for in k/k... allow the release team to track whether a given issue was actually being worked. The main difference is that /status commands are gated behind team membership. I'm not a fan of people having to /status in-progress stuff because it feels like process scarring and history shows it's laughably not updated: 285 closed issues are supposedly "in progress", and 4/11 open issues haven't been updated in over 3 weeks.

That said, if active works for you as a human nicety, I'd rather nuke status/in-progress and add support for (but not mandate) /lifecycle active. What I would not want is something sweeping through and adding /lifecycle active to issues updated within N days and generating more label noise.

The milestone-maintainer munger consumes status/in-progress and I would want it to use lifecycle/active instead, if we were going to use it (or... just drop the label entirely from its requirements)

I've added to tomorrow's sig-contribex agenda to get feedback

I'd also appreciate input from @kubernetes/sig-release-feature-requests

neolit123 commented 6 years ago

thanks for the comments and bringing this to contribex, @spiffxp.

@stevekuznetsov

Is this a proposal to add this to all PRs until lifecycle/stale is needed

we didn't discuss PRs. right after submission a PR is obviously active, so it's valid to have the label. it's becomes a question if the label should be auto/explicit on all new PRs. that might be a bit too much...

Or just to support the user command /lifecycle active?

i think /lifecycle active is a must and it should be manual. the bots should remove it if the issue or PR hasn't been active for a while and apply lifecycle/stale.

BenTheElder commented 6 years ago

I think /lifecycle active is a must and it should be manual. the bots should remove it if the issue or PR hasn't been active for a while and apply lifecycle/stale.

This was my thinking earlier, it's an optional label but supported by the lifecycle commands and the lifecycle bot removes it. It would just be an extra positive label like it is now, but with support to add it via command without needing write access, and for the bot to handle it with the rest of issue lifecycle.

On Jul 24, 2018 17:15, "Lubomir I. Ivanov" notifications@github.com wrote:

thanks for the comments and bringing this to contribex, @spiffxp https://github.com/spiffxp.

@stevekuznetsov https://github.com/stevekuznetsov

Is this a proposal to add this to all PRs until lifecycle/stale is needed

we didn't discuss PRs. right after submission a PR is obviously active, so it's valid to have the label. it's becomes a question if the label should be auto/explicit on all new PRs. that might be a bit too much...

Or just to support the user command /lifecycle active?

i think /lifecycle active is a must and it should be manual. the bots should remove it if the issue or PR hasn't been active for a while and apply lifecycle/stale.

— You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/8811#issuecomment-407592253, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4BqwmDt6DX9F7XNynilEWNPluMtliGks5uJ7ilgaJpZM4VdMYI .

jberkus commented 6 years ago

My $0.02 as ex-release lead:

  1. I'm OK with status/in-progress going away, and being removed from the milestone munger bot. People aren't using it and it's not serving its intended purpose.
  2. adding lifecycle/active should require the issue/PR to be assigned. So (a) if nobody is assigned, it should auto-assign to the person applying the label (if they're an or member), and (b) if the issue becomes un-assigned, lifecycle/active should be auto-removed.
  3. to limit "cookie licking" problems, the lifecycle/active label should be auto-removed if there's no activity on the issue/PR within a certain number of days (ideally 4 days during code freeze, and 9 days otherwise). This is trickier than it sounds, though, because for projects like kubeadm there may be an "active" issue in k/k whereas the actual work is happening in k/kubeadm.
  4. Lifecycle/active should not mean anything to the milestone munger bot. It should be strictly for human information and searches.
cblecker commented 6 years ago

@jberkus What about the use case where the person working on an issue isn't an org member? Difficult to assign. (we had the same thought around the help wanted label)

jberkus commented 6 years ago

Yes, that's why I'm dubious about that use-case. I understand the desire for it, but it's very difficult to make it work. The alternative, not requiring assignment, means that we have a tag indicating that an issue is being worked on but we don't know by whom. How is that useful?

jberkus commented 6 years ago

An alternative for new contributors would be to assign the issue to a "mentor" in the appropriate SIG. This would also show that there's some communication with the SIG. @guineveresaenger, thoughts?

cblecker commented 6 years ago

There's merits to that, but I wouldn't want to block a new contributor working on something based on mentor bandwidth.

guineveresaenger commented 6 years ago

Agree with @cblecker - also we'd need to devise a process to find a mentor.

Regarding the label being activated for different lengths during code freeze vs. otherwise, I'm not even sure if this applies to a specific use case - don't we already have a "needs-attention" label?

jberkus commented 6 years ago

@cblecker so, we have an issue, and it's been marked "lifecycle/active", it's in the milestone, and there's nobody assigned to it.

How are we supposed to figure out who's actually working on it?

neolit123 commented 6 years ago

right now what i'm doing is the moment i manually apply the label i also mention the github user who is working on the issue (e.g. user-xx is working on this). the last time i did that the github user was a non-k8s-org member.

jberkus commented 6 years ago

Can we mandate mention of a user by bot?

stevekuznetsov commented 6 years ago

we didn't discuss PRs.

So do you want to add labels to the issue based on:

Sorry if I am not understanding the explanation but it is still a little confusing.

BenTheElder commented 6 years ago

The initial request here is just for a manual command to add an "active" label, instead of doing so with edit permissions through the github UI. The secondary ask is that perhaps the lifecycle bot should be aware of this, and remove it as the issue goes stale.

Imo, this is totally reasonable. If a repo wants to use this label they can search for the label etc., but the bot will manage removing it and proxy permission to apply it.

Using it to also replace status/in-progress for k/k and possibly enforcing the circumstances under which it is applied might make sense, but imo can reasonably be sorted out in follow-ups / a seperate discussion. Just making another label usable without direct editing should be useful.. as would life-cycling it away eventually.

neolit123 commented 6 years ago

@stevekuznetsov

So do you want to add labels to the issue based on:

for issues, someone is working on fixing this issue and a PR will be sent soon or was already sent. also indicates to everyone that this issue is taken.

we didn't discuss PRs.

for PRs, this is less useful. a PR is supposed to be active once submitted (rebases, CI babysitting, code review attention, etc), which makes the usage of the label for PRs redundant but still valid - i.e. not incorrect even if auto-applied.

potential description: Denotes an issue or PR that is actively worked on.

neolit123 commented 6 years ago

@jberkus

Can we mandate mention of a user by bot?

it could be a convenient extension.

tpepper commented 6 years ago

@neolit123 I think Steve meant not adding this label to PRs but rather does associated PR imply “active”?

neolit123 commented 6 years ago

@tpepper if there is an obvious PR that fixes and issue and both xref, then the issue active state is implied as PRs have an implied state of active until stale. :roller_coaster:

without the label:

tpepper commented 6 years ago

Ah hadn't thought about the issue of unrelated PR. Definitely then does NOT make sense to have PR xref auto trigger 'active' label addition.

spiffxp commented 6 years ago

Using it to also replace status/in-progress for k/k and possibly enforcing the circumstances

Yeah I agree... maybe I spiraled us out of scope by raising the overlap. I'm fine just moving forward with /lifecycle active, and altering the existing periodic-test-infra-stale job. Bikeshedding over enforcement is exactly where I don't want to go

cblecker commented 6 years ago

Opened #8865 :)

We can look at the status/ class of labels later.

neolit123 commented 6 years ago

@cblecker i've tried the label today, but didn't seem to work. :) https://github.com/kubernetes/kubeadm/issues/1030#issuecomment-410114729

edit: Ben answered in there.