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.

Sign up for GitHub

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

Jump to bottom

Bot not applying the right label sometimes for PRs #1099

Closed
ramya-rao-a opened this issue Oct 15, 2020 · 30 comments
Closed

Bot not applying the right label sometimes for PRs #1099

ramya-rao-a opened this issue Oct 15, 2020 · 30 comments
Assignees
@praveenkuttappan
Labels
Central-EngSys This issue is owned by the Engineering System team. EngSys This issue is impacting the engineering system.
Milestone
Backlog

Comments

@ramya-rao-a
Copy link

PRs in the azure-sdk-for-js repo for Metrics Advisors and Communication packages are not getting the right label applied to them even if they are mapped as expected in the CodeOwners file

Examples:
@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Oct 15, 2020
@weshaggard weshaggard added the EngSys This issue is impacting the engineering system. label Oct 16, 2020
@weshaggard
Copy link
Member

I believe this is because we haven't refreshed the data from the codeowners files. I know @AlexGhiondea was planning to manually run it again but we should also have an issue (perhaps this one) to track running this on some cadence. @mitchdenny do we have anything else that is watching codeowners files that we could add this to? Or should we just use some scheduling system (DevOps or function) that will run it on some schedule?

@mitchdenny
Copy link
Contributor

I know that the live test notifications hang off the CODEOWNERS file as well. How does the bot pick up its configuration @AlexGhiondea?

@weshaggard
Copy link
Member

I know that the live test notifications hang off the CODEOWNERS file as well.

That is done by a daily Devops Pipeline that runs https://dev.azure.com/azure-sdk/internal/_build?definitionId=679. We could do something similar here or maybe create a combined one.

@kurtzeborn
Copy link
Member

This appears to have been caused because the bot doesn't "refresh" after a change is made to CODEOWNERS files. Let's use this issue to track implementing that refresh.

@kurtzeborn kurtzeborn added Central-EngSys This issue is owned by the Engineering System team. and removed needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels Oct 29, 2020
@mitchdenny
Copy link
Contributor

@weshaggard yep ... I think it would make sense to create a combined job here for refreshing anything that is tied to the CODEOWNERS file. @AlexGhiondea is there a tool that you already have that refreshes the bot settings?

@ramya-rao-a
Copy link
Author

Can we have the bot "refreshed" manually for now and then continue to use this issue to track anything else we want to do? I have a lot of incoming PRs with no labels that I have to manually go and apply labels now

@mitchdenny mitchdenny added this to the Backlog milestone Nov 3, 2020
@mitchdenny
Copy link
Contributor

I've done the manual refresh for now.

sima-zhu pushed a commit to sima-zhu/azure-sdk-tools that referenced this issue Dec 3, 2020
@ahsonkhan
Fix the strict-aliasing issue in _az_isfinite. (Azure#1099)
11996e7 
* Fix the strict-aliasing issue in _az_isfinite.

* Use memcpy as a workaround for strict aliasing warning.

* Add some more comments and a precondition check on size.

* Remove precondition check since the sizeof comparison was constant.

* Fix similar issue in tests.
@AlexGhiondea
Copy link
Contributor

@mitchdenny are you asking about this?

If so, then yes, you can point that tool to the code owners file in each of the repos and it will update the configuration for the bot. I ran this before the holidays as there were some other changes that need to be refreshed.

@mitchdenny
Copy link
Contributor

Yeah ... its just something that needs to be done regularly. I see you reached out to the FabricBot team to ask about alternative authentication schemes (I did as well, and got the same response).

@ramya-rao-a
Copy link
Author

The manual refresh didnt seem to stick. See Azure/azure-sdk-for-js#13121 which should have gotten the Mgmt label, but didnt

@ramya-rao-a
Copy link
Author

Alright, am re-opening this

The below PRs should have gotten the Mgmt label, they didnt:

@ramya-rao-a ramya-rao-a reopened this Jan 25, 2021
@ramya-rao-a
Copy link
Author

This is driving me nuts! Please help!
Another set of PRs that should have gotten the Mgmt label, and they didnt

@mitchdenny
Copy link
Contributor

mitchdenny commented Apr 26, 2021 •
edited
Loading

Looks like the various arm-* rules failed because fabric bot doesn't know how to interpret them:

image

So we've got two feature asks on the Fabric Bot team:

  1. service principal based authentication (or repo-based configuration)
  2. support for globbing syntax in filters.

Without 1, someone on the EngSys team has to go in every single time a rule is updated. And without 2 we have to have a separate rule (no globs) for each of the arm-* folders (which exacerbates the first issue).

@AlexGhiondea
Copy link
Contributor

I believe they are working on a better auth story.

@mitchdenny
Copy link
Contributor

That isn't the last I heard so that is good news. Did the person you spoke to give you any kind of ETA (better auth also implies they are acknowledging the need for a supported API?)

@ramya-rao-a
Copy link
Author

@praveenkuttappan Can we have the manual refresh done in a regular frequency? We have more services joining our repos and the codeowners files are getting updated, and the bot is not able to keep up

@praveenkuttappan
Copy link
Member

@AlexGhiondea Do you have any wiki or any info on how to trigger this labeling bot manually?

@weshaggard
Copy link
Member

IIRC the main issue is that the fabric bot didn't provide a proper rest API we could call so Alex hacks around it by using a local cookie to try and automate this but that isn't a good way to do this automatically. It is probably worth reaching out the the fabric bot folks to see if they have made any headway on supporting an api for updating the rules.

@AlexGhiondea
Copy link
Contributor

What @weshaggard said is right. I have a tool that can update the rules for the bot, but it requires that we extract an auth token from a cookie. @praveenkuttappan I can show you how to do it if you want to give it a try!

This was referenced Aug 24, 2021
Merged
Merged
ghost pushed a commit to Azure/azure-sdk-for-js that referenced this issue Aug 24, 2021
@ramya-rao-a
Update CODEOWNERS for every mgmt package in feature/v4 branch (#17061)
2139591 
As discussed in Azure/azure-sdk-tools#1099, usage of glob patterns does not work for our bot. So adding codeowner for each of the mgmt plane package. I copied the list from https://github.com/Azure/azure-sdk-for-js/blob/feature/v4/eng/pipelines/mgmt-ci.yml

This was done in the main branch in #17060

Repeating it in the feature/v4 branch as this branch is used to make updates to the older versions of the mgmt plane packages
@ramya-rao-a
Copy link
Author

Hello (again)

I have given up waiting for a solution here and have added an entry for each mgmt plane package in the JS repo directly. Please see Azure/azure-sdk-for-js#17060

Can we get the "refresh" done so that the bot starts applying the right labels?

@praveenkuttappan
Copy link
Member

@AlexGhiondea I would like to know more about this tool so I can try it. I can also reach out to fabric bot team once I understand more about what/how we do it now.

@ramya-rao-a
Copy link
Author

How is that refresh going?

@ramya-rao-a
Copy link
Author

Not sure what happened... now none of the PRs are getting any labels :(

@AlexGhiondea
Copy link
Contributor

@ramya-rao-a the refresh went fine... but now there aren't any labels associated with any paths... I'm reaching out to the bot team to see what's going on 🙂.

@ramya-rao-a
Copy link
Author

This is me, just posting here, like I do every 2 weeks, wondering what to expect next... None of the bot labelling is working now :(

@praveenkuttappan
Copy link
Member

@ramya-rao-a Can we close this now? I know we still don't have any automation to refresh the labels since we can't authenticate as bot to fabric bot and needs out login token to trigger it and regular manual refresh is still the only solution.

@ramya-rao-a
Copy link
Author

What is the plan for manual refresh?

@benbp
Copy link
Member

benbp commented Nov 3, 2021

@praveenkuttappan has anyone considered contributing SP auth to the fabric bot?

@kurtzeborn
Copy link
Member

Pretty sure this no longer matters now that we've moved away from FabricBot.

CC: @JimSuplizio

@github-project-automation github-project-automation bot moved this from 🤔 Triage to 🎊 Closed in Azure SDK EngSys 🤖🧠 Nov 6, 2023
@JimSuplizio
Copy link
Member

@kurtzeborn, you're right, if the CODEOWNERS file is correct the labels should be being applied. The problem was that FabricBot didn't directly use the CODEOWNERS file. There was a script that generated the FabricBot.json from the CODEOWNERS which apparently broke and they stopped using it and started updating CODEOWNERS files directly. Moving to actions and the github-event-processor now uses the CODEOWNERS files.

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

@praveenkuttappan praveenkuttappan

Labels
Central-EngSys This issue is owned by the Engineering System team. EngSys This issue is impacting the engineering system.
Projects
Azure SDK EngSys 🤖🧠
Archived in project
Milestone
Backlog
Development

No branches or pull requests
8 participants
@mitchdenny @benbp @AlexGhiondea @weshaggard @JimSuplizio @ramya-rao-a @kurtzeborn @praveenkuttappan