tensorflow / community

Stores documents used by the TensorFlow developer community
Apache License 2.0
1.26k stars 576 forks source link

Contributors are still not notfied after a PR rollback #425

Open bhack opened 2 years ago

bhack commented 2 years ago

Contributors are still not notified when we are going to rollback PR merge. Also in the case we have a linked issue to the PR it is not reopened or commented by the bot.

Contributors still thinks that their contributions was merged without any other notification.

Just an example: https://github.com/tensorflow/tensorflow/pull/57478

/cc @theadactyl @joanafilipa @learning-to-play @gbaned @mdanatg

mihaimaruseac commented 2 years ago

Seems to be caused by there being a PR being created to a wrong branch: https://github.com/tensorflow/tensorflow/pull/57012

mihaimaruseac commented 2 years ago

Actually, here is the action that ran to create rollback PR:

https://github.com/tensorflow/tensorflow/actions/runs/2784555032

This is weird

Error: Unhandled error: HttpError: Validation Failed: {"value":"bhack","resource":"Issue","field":"assignees","code":"invalid"}
mihaimaruseac commented 2 years ago

Next time the action worked we got it finished successfully: tensorflow/tensorflow#56572 rollback caused tensorflow/tensorflow#57115

Action is https://github.com/tensorflow/tensorflow/actions/runs/2843891794

bhack commented 2 years ago

I didn't read the action source code but your last case seems different as it was not exactly a merge on the PR commit.

bhack commented 2 years ago

@mihaimaruseac I read the Action code and the log a bit but I cannot mention the author as the commits related to that Github Action contribution was masqueraded by the @tensorflower-gardener "anon user".

I guess that the difference between your example and mine is that assigning the ticket also to me, or other contributors, with my role/permission is going to create an error.

https://docs.github.com/en/issues/tracking-your-work-with-issues/assigning-issues-and-pull-requests-to-other-github-users

Probably we could add a preliminarily check for all the users involved if they could be assigned: https://docs.github.com/en/rest/issues/assignees#check-if-a-user-can-be-assigned

If not they need to be mentioned /cc @<user> in the body of the ISSUE creation API request.

bhack commented 2 years ago

To summarize:

You can assign multiple people to each issue or pull request, including yourself, anyone who has commented on the issue or pull request, anyone with write permissions to the repository, and organization members with read permissions to the repository.

  1. @mihaimaruseac Can you check if I still have read access to the TF repo?

  2. More in general, for the "unknown team member" that contributed that Github Action, his assumption is wrong. PR author/owner cannot be considered automatically as assignee.

mihaimaruseac commented 2 years ago

Thanks for the debugging. I agree that we should probably do CC @user inside the body of the issue.

CC @learning-to-play to handle this

bhack commented 2 years ago

Just a note.

Point 1. was caused by a permission incident we had also on my account. Now the action runned fine: https://github.com/tensorflow/tensorflow/runs/8075237000?check_suite_focus=true

Point 2. It is a bug so it still need to be fixed as not all PR submitters are team members.

bhack commented 1 year ago

Please note that also PR reviewers are not notified on the rollback:

https://github.com/tensorflow/tensorflow/issues/58332#issuecomment-1295298881

bhack commented 1 year ago

The rollback doesn't reopen the connected ticket:

https://github.com/tensorflow/tensorflow/issues/59076

mihaimaruseac commented 1 year ago

It needs to go through multiple hops to get that information, the best compromise was to create a new issue in which to tag reviewer and PR author

bhack commented 1 year ago

We need to notify the PR approver: https://github.com/tensorflow/tensorflow/issues/59078

bhack commented 1 year ago

/cc @MichaelHudgins

mihaimaruseac commented 1 year ago

PR approver is notified internally on the rollback. Not that easy to get it on Github side from Copybara.

bhack commented 1 year ago

It seems to me that almost all transparency/limits gaps have some source in copybara/jobs.

If, it seems that we could/would switch to a GitHub first development model in some core ex-tensorflow components there is hope that we could achieve this in also in TF one of these days ("Velle est posse").