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

Contributors are still not notfied after a PR rollback #425

Open
bhack opened this issue Aug 27, 2022 · 15 comments
Open

Contributors are still not notfied after a PR rollback #425

bhack opened this issue Aug 27, 2022 · 15 comments

Comments

@bhack
Copy link
Contributor

bhack commented Aug 27, 2022

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:
tensorflow/tensorflow#57478

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

@mihaimaruseac
Copy link
Contributor

Seems to be caused by there being a PR being created to a wrong branch: tensorflow/tensorflow#57012

@mihaimaruseac
Copy link
Contributor

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
Copy link
Contributor

mihaimaruseac commented Aug 27, 2022

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
Copy link
Contributor Author

bhack commented Aug 28, 2022

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
Copy link
Contributor Author

bhack commented Aug 28, 2022

@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
Copy link
Contributor Author

bhack commented Aug 29, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

bhack commented Aug 29, 2022

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
Copy link
Contributor Author

bhack commented Oct 28, 2022

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

tensorflow/tensorflow#58332 (comment)

@bhack
Copy link
Contributor Author

bhack commented Jan 3, 2023

The rollback doesn't reopen the connected ticket:

tensorflow/tensorflow#59076

@mihaimaruseac
Copy link
Contributor

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
Copy link
Contributor Author

bhack commented Jan 4, 2023

We need to notify the PR approver:
tensorflow/tensorflow#59078

@bhack
Copy link
Contributor Author

bhack commented Jan 4, 2023

/cc @MichaelHudgins

@mihaimaruseac
Copy link
Contributor

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

@bhack
Copy link
Contributor Author

bhack commented Jan 5, 2023

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").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants