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

Update the way Service Labels are matched #7794

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

JimSuplizio
Copy link
Member

@JimSuplizio JimSuplizio commented Feb 29, 2024

This is being done as per requirements from @jsquire

This applies to the new InitialIssueTriage rule (when it's activated) as well as the ServiceAttention rule. In either case, we have a list of labels either coming from the AI Label Service or from the Issue, itself, when Service Attention is added. Using the aforementioned list, we'll loop through the CodeownersEntries, from the bottom up, to find the first entry whose ServiceLabels are all inclusive in the list.
Here's an simplistic example. Say we have the following CODEOWNERS entries (I'm only listing a couple of ServiceLabel pieces for brevity.

#ServiceLabels: %Storage
#ServiceLabels: %Mgmt
#ServiceLabels: %Mgmt %Storage

If the list of labels contained %Mgmt %Storage. Looking from the bottom up, we'd match the entry whose ServiceLabels has both Mgmt and Storage. If the list of labels contained %Mgmt %Foo, the match would be the entry whose ServiceLabel only contained Mgmt. If the list of labels contained %Client %Storage, it would match the entry whose ServiceLabel contained %Storage. If the list of labels contained %Client %Foo it wouldn't match anything.

Tests were added for these new scenarios.

There is one big change here and that's the ServiceAttention rule. Back in November of 2023, a change was made to allow processing of the ServiceAttention rule regardless of the order that labels were applied. This change has to be removed. @jsquire and I discussed this and because labels need to be processed in a set, ServiceAttention has to be added after the labels are added. Otherwise, this change to matching would end matching singular entries for each label as it was being added which would result in incorrect processing. In the above example, with the current rule's processing, if ServiceAttention was already on the Issue and it was supposed to go to %Mgmt %Storage, when the user added the first label, Mgmt OR Storage, it would match the ServiceLabel entry with the singular label. This would result in ServiceAttention rule processing when the first label is added and then again when the second one is added and, in both cases, the processing would be wrong.

Also, rather than have a big chunk of commented out code in the InitialIssueTraige function, which makes it painful when trying to update something for a rule that's enabled yet. The rule for InitialIssueTriage, when AzureSdkOwners are defined, is commented out in between TODO tags with instructions on what do to when it's time to enable it. The existing, interim rule's function would be deleted at that time.

There was one minor change: LabelConstants was too ubiquitous and renamed to TriageLabelConstants to better reflect what these actually are. These are all triage labels. They're added by rules when processing or they cause rules to process or both.

@JimSuplizio JimSuplizio added the GitHub Event Processor Anything related to the GitHub Event Processor label Feb 29, 2024
@JimSuplizio JimSuplizio self-assigned this Feb 29, 2024
@JimSuplizio JimSuplizio requested a review from benbp as a code owner February 29, 2024 20:52
@JimSuplizio
Copy link
Member Author

@jsquire,
@weshaggard has a good point with these changes. No labeled events can actually process Service Labels without some "last label added triggers the processing" which is how the Service Attention processes today.

@jsquire
Copy link
Member

jsquire commented Feb 29, 2024

@jsquire, @weshaggard has a good point with these changes. No labeled events can actually process Service Labels without some "last label added triggers the processing" which is how the Service Attention processes today.

Not sure that I'm following.

@JimSuplizio
Copy link
Member Author

@jsquire, @weshaggard has a good point with these changes. No labeled events can actually process Service Labels without some "last label added triggers the processing" which is how the Service Attention processes today.

Not sure that I'm following.

The reason why we had to revert the relaxed Service Attention rule is because you're wanting to match CODEOWNERS ServiceLabel entries off of two labels. Meaning that these labels have to be added and then Service Attention has to be added in order to correctly get the ServiceLabel match in CODEOWNERS. Service Attention is the only rule like this (Initial Issue Triage gets its list from the AI Label Service). If there's another rule that needs to processes Service Labels on an issues labeled event, it needs to follow the same pattern as the Service Attention.

@jsquire
Copy link
Member

jsquire commented Feb 29, 2024

@jsquire, @weshaggard has a good point with these changes. No labeled events can actually process Service Labels without some "last label added triggers the processing" which is how the Service Attention processes today.

I'm concerned about some of the smaller repositories that can't be trained directly for AI models, though I suppose that we can just remove them from the label service entirely which would force them into the "needs-triage" flow.

@jsquire
Copy link
Member

jsquire commented Feb 29, 2024

@jsquire, @weshaggard has a good point with these changes. No labeled events can actually process Service Labels without some "last label added triggers the processing" which is how the Service Attention processes today.

Not sure that I'm following.

The reason why we had to revert the relaxed Service Attention rule is because you're wanting to match CODEOWNERS ServiceLabel entries off of two labels. Meaning that these labels have to be added and then Service Attention has to be added in order to correctly get the ServiceLabel match in CODEOWNERS. Service Attention is the only rule like this (Initial Issue Triage gets its list from the AI Label Service). If there's another rule that needs to processes Service Labels on an issues labeled event, it needs to follow the same pattern as the Service Attention.

Yup, agreed. The only other ServiceLabel match rule is automated triage, and the entire set of issue labels is known upfront. I see these as special cases, not something that we'd want to drive more general functionality from.

@JimSuplizio JimSuplizio merged commit 2d364d5 into Azure:main Feb 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GitHub Event Processor Anything related to the GitHub Event Processor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants