This message was deleted.
# bug-reportss
Slackbot
05/25/2023, 1:18 PMThis message was deleted.
r
Ricardo Delfin
05/25/2023, 1:51 PMI've had to merge this PR but this seems to be happening on all my PRs today
j
Jacob Gold
05/25/2023, 3:02 PM@Brendan Ngo , can you take a look?
b
Brendan Ngo
05/25/2023, 3:12 PMI'll take a look!
Brendan Ngo
05/25/2023, 3:25 PMhm looks like you've merged the pr already so we've lost that state
Brendan Ngo
05/25/2023, 3:28 PMwe've started calculating required codeowners on Graphite which is why you're seeing that message now. If you see this again can you let me know? It may have been a problem with us missing some data on our side at the time. On our side now it does look like the pr has all owners.
Brendan Ngo
05/25/2023, 3:29 PMalso question, how was reviewing on Graphite with codeowners required before? we didn't respect it at all so I'm sure our mergeability calculations were never right for you?
r
Ricardo Delfin
05/26/2023, 1:30 PMOh! Really sorry. Yeah, I'll ping you next time I see it happen
Ricardo Delfin
05/26/2023, 1:30 PMBefore, it just didn't let me merge I thought, though I might have missed it letting me. I usually monitor who's approved before I press merge
Ricardo Delfin
05/26/2023, 1:31 PMThat said... We also have a bot that tells you the minimum set of reviewers you need for a PR (codeowners get... Complicated real fast, and our codeowners file is quite large and uses some weird github behaviour related to how they evaluate codeowners)
b
Brendan Ngo
05/26/2023, 4:15 PMbefore, if you had minimum number of approvals you could try to merge on graphite but if we were missing required codeowners it would fail doing the merge, which is the reason we wanted to start enforcing codeowners on the graphite side. codeowners does get complicated quickly, especially for prs with lots of files with lots of different codeowners. Hopefully our new codeowner checks work for you but definitely let us know if you see something weird again!
r
Ricardo Delfin
05/30/2023, 11:19 AMOk, I've got another PR
Ricardo Delfin
05/30/2023, 11:19 AMThat I can merge in github but not graphite
b
Brendan Ngo
05/30/2023, 5:05 PMOh I see the problem now! Looks like our codeowners calculation is right but we don't have data for those teams so we couldn't link the reviewers to the team codeowners. We should have this data so looking into why it's missing!
Brendan Ngo
05/30/2023, 7:57 PMThere's a fix going out now that should fix this going forward! Thanks for flagging and let me know if you see anything unexpected still
r
Ricardo Delfin
05/31/2023, 8:32 AMOooh that's interesting! Is it just that you don't have access to our owners groups int the integration? But thanks for looking into this!
b
Brendan Ngo
05/31/2023, 3:16 PMwe should have permission there was just a bug in our teams logic. codeowners is also tricky since there isn't an api for it so we need to rederive everything ourselves
e
Eitan Levi
06/08/2023, 4:15 PMMy apologies for "reopening" this thread, but I'm still experiencing this problem - I'm not sure if it's the same problem or if it's just similar/related.
We auto-assign two reviewers to each PR, but we only require approval from a single code-owner before merging.
What happens is: one reviewer approves, and I'm able to merge in GitHub - but Graphite still shows:
Needs approvals from code ownersb
Brendan Ngo
06/08/2023, 5:32 PM@Eitan Levi do you have an example pr where this is happening?
e
Eitan Levi
06/08/2023, 5:47 PMI'll get one set up - just waiting for the CI checks to finish.
b
Brendan Ngo
06/08/2023, 6:00 PMfeel free to dm when you do and I can take a look!
e
Eitan Levi
06/08/2023, 6:19 PMawesome, thanks - DM's you
2 Views