no issue created when clippy toolstate breaks

[matthiaskrgr]

#61672 broke clippy toolstate but there was no issue created automatically.

Probably linked to rls/clippy decoupling #61670

[matthiaskrgr]

@rustbot modify labels: T-dev-tools T-infra

[ehuss]

I don't think it is related to rls. clippy-driver has never opened an issue when it has broken in the past.

The logs say I/O error: HTTP Error 422: Unprocessable Entity which is GitHub's error message for "Sending invalid fields". Unfortunately the script doesn't print the response body.

I can't see anything unusual in the script to explain why it would fail only for clippy-driver. My best guess is that there is something wrong with the list of assignees. The API docs say you can assign up to 10, so the larger number shouldn't be a problem.

@kennytm I could add some more debug output to publish_toolstate.py to understand the 422 error. I could also try to run the code locally with my own github token to see what the error is, but I don't want to generate noise pinging people if it succeeds.

[kennytm]

Apparently the GitHub API docs is wrong, unassignable users are causing the 422 errors instead of being silently dropped as documented.

No issues for clippy-driver because @llogiq, @mcarton and @phansch can't be assigned.

I think rustfmt will have the same issue because @topecongiro can't be assigned.

Additionally, no issues for #62003 because the assignees array included the PR author @christianpoveda, which also can't be assigned.

[kennytm]

(Re-nominating for what to do with the assignees field. )

[kennytm]

In yesterday's infra meeting, we have decided to fix this issue by separating the list of people to be assigned and mentioned, and also the PR author will no longer be assigned automatically.

This could be implemented by

1. Copy the MAINTAINERS configuration into a new dictionary of lists of usernames ASSIGNEES containing only the assignable members:

   rust/src/tools/publish_toolstate.py

   Lines 16 to 31 in d3e2cec

   	# List of people to ping when the status of a tool or a book changed.
   	MAINTAINERS = {
   	'miri': '@oli-obk @RalfJung @eddyb',
   	'clippy-driver': '@Manishearth @llogiq @mcarton @oli-obk @phansch',
   	'rls': '@Xanewok',
   	'rustfmt': '@topecongiro',
   	'book': '@carols10cents @steveklabnik',
   	'nomicon': '@frewsxcv @Gankro',
   	'reference': '@steveklabnik @Havvy @matthewjasper @ehuss',
   	'rust-by-example': '@steveklabnik @marioidival @projektir',
   	'embedded-book': (
   	'@adamgreig @andre-richter @jamesmunns @korken89 '
   	'@ryankurte @thejpster @therealprof'
   	),
   	'edition-guide': '@ehuss @Centril @steveklabnik',
   	}

2. Do not compute assignees in issue(), instead, accept another argument into the function

   rust/src/tools/publish_toolstate.py

   Lines 65 to 75 in d3e2cec

   	def issue(
   	tool,
   	status,
   	maintainers,
   	relevant_pr_number,
   	relevant_pr_user,
   	pr_reviewer,
   	):
   	# Open an issue about the toolstate failure.
   	assignees = [x.strip() for x in maintainers.split('@') if x != '']
   	assignees.append(relevant_pr_user)

3. Obtain the assignees here

   rust/src/tools/publish_toolstate.py

   Lines 163 to 166 in d3e2cec

   	issue(
   	tool, new, MAINTAINERS.get(tool, ''),
   	relevant_pr_number, relevant_pr_user, pr_reviewer,
   	)

[ehuss]

I'm curious if it is necessary to set the assignee field at all. If all the people are @ mentioned in the body, they should be sufficiently notified. Does setting the assignee field have any benefits that I'm not aware of? It seems like skipping all this would simplify it a lot. These issues typically get fixed very quickly anyways.

[kennytm]

Assignees can reach the issues again from https://github.com/issues/assigned, but for mentions it will be gone once the notification is read.

[matthiaskrgr]

Is there a reason for keeping the issue open?
I think this might be fixed? cc #62338

[RalfJung]

#62023 implemented the strategy that the author of the breaking PR does not get assigned any more. So creating issues should always work now.

However, we still assume all people listed in MAINTAINERS can be assigned. That seems reasonable; the list is manually curated and at least historically adding people to some team was not a high bar.

So, closing this issue as fixed.

[ehuss]

we still assume all people listed in MAINTAINERS can be assigned

I don't think this is true. There are people in MAINTAINERS that cannot be assigned. For example, for clippy llogiq and mcarton are not members of this repo. Given that, I'm a bit baffled how #62338 got opened. It seemed to assign the first two people, and then ignored the rest?

[RalfJung]

Oh. That sounds like fun.

Any reason they can't be added so the org so that they can be assigned? Seems reasonable that people maintaining tools important enough to be tracked this way are added to the org (AFAIK that doesn't even automatically entail any new powers).

[RalfJung]

Otherwise my proposal would be to make MAINTAINERS a dictionary mapping tools to a pair of strings; the first will be assigned and the second just mentioned. That seems nicer to me than what @kennytm proposed, which would duplicate the tool list.

[RalfJung]

I think this has been resolved by #64119.