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

Support for reviewers & assignees #271

Merged
merged 6 commits into from
Nov 29, 2024
Merged

Conversation

StefMa
Copy link
Contributor

@StefMa StefMa commented Nov 28, 2024

fixes #215

This time the forgotten reviewers and assignees 🙂

Haven't tested it yet ...

@StefMa StefMa force-pushed the reviewer-assignees branch from 54dcd9d to db4de90 Compare November 28, 2024 09:46
Copy link
Member

@alextu alextu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! LGTM, just suggesting a small refactoring and we can merge it once you confirm it works properly (I'll test it too)

@StefMa StefMa force-pushed the reviewer-assignees branch from c16ccc6 to 8ef4d4a Compare November 28, 2024 15:29
Signed-off-by: Stefan M. <[email protected]>
@StefMa StefMa force-pushed the reviewer-assignees branch from 8ef4d4a to d4cbfb9 Compare November 28, 2024 15:36
@StefMa
Copy link
Contributor Author

StefMa commented Nov 28, 2024

once you confirm it works properly

I can confirm that "it works".

Assignees works perfectly.
See StefMa#6

I “couldn’t test” reviewers. First, I tried to put you as a reviewer, then I got the message from GitHub (via the GitHub API project) that you can only add collaborators as reviewers. Then I set myself as a reviewer, but that didn’t work either. The message was that you can’t set the reviewer to the same person as the author.

Because those errors indicate that there are API calls happening, I would assume that everything works as expected.

However, those errors throw an exception and therefore failed the build. Maybe we want to catch those and report them only via the logger? 🤔 What do you think?

@alextu
Copy link
Member

alextu commented Nov 28, 2024

once you confirm it works properly

I can confirm that "it works".

Assignees works perfectly. See StefMa#6

I “couldn’t test” reviewers. First, I tried to put you as a reviewer, then I got the message from GitHub (via the GitHub API project) that you can only add collaborators as reviewers. Then I set myself as a reviewer, but that didn’t work either. The message was that you can’t set the reviewer to the same person as the author.

Because those errors indicate that there are API calls happening, I would assume that everything works as expected.

However, those errors throw an exception and therefore failed the build. Maybe we want to catch those and report them only via the logger? 🤔 What do you think?

I've tested it as well and noticed the same, I'll be able to test it end to end tomorrow. I agree that we should not fail the build since the PR is created, and just output a warning message in that case.

@StefMa
Copy link
Contributor Author

StefMa commented Nov 29, 2024

I changed the code a bit and cleaned it up.
Check it out here: a2a408e

  • Add try-catch around addAssignees and requestReviewers and addLabels
  • In case of an error, we just log it as warning
  • Also I moved the parts after the output of "PR created", because actually this happens afterwards
  • I extract methods for each of them

I tested the case of "request review for myself (author)" and the build succeeded, but the error is visible:
Screenshot 2024-11-29 at 8 29 03 AM

I tested to add a label that doesn't exist to check if an error happen.
But actually it just creates that label 🫠
See StefMa#8 🤷
Anyways, according to the method signature it could also throw an IOException.
So better safe than sorry I would say.

@StefMa StefMa requested a review from alextu November 29, 2024 07:31
@alextu
Copy link
Member

alextu commented Nov 29, 2024

Thanks! I've tested it with reviewers as well, looks good, I'll merge the PR soon

@alextu alextu merged commit 8a00d4d into gradle:main Nov 29, 2024
6 checks passed
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

Successfully merging this pull request may close these issues.

Add options to specify labels, reviewers and assignees to be associated with the created pull request
2 participants