search

chevah / github-hooks-server

Handling of GitHub hooks for Chevah project
BSD 3-Clause "New" or "Revised" License
1 stars 1 forks source link

[#33] Address Twisted needs #43

Closed danuker closed 2 years ago

danuker commented 2 years ago

Fixes #33

TODO:

Not implemented:

reviewers: @adiroiban

The new version was deployed manually (instructions in README.rst). Feel free to test!

danuker commented 2 years ago

Testing attempt to assign @octocat

needs-review

danuker commented 2 years ago

needs-changes

Never mind, the new bot is not deployed yet.

danuker commented 2 years ago

please review testing

danuker commented 2 years ago

needs-review

testing

danuker commented 2 years ago

please review testing

danuker commented 2 years ago

please review testing again

danuker commented 2 years ago

please review

did not have a default configured here; added team for review

danuker commented 2 years ago

Error here:

Traceback (most recent call last):
File "/home/site/wwwroot/chevah/github_hooks_server/server.py", line 81, in hook
response = handle_event(event)
File "/home/site/wwwroot/chevah/github_hooks_server/server.py", line 144, in handle_event
return handler.dispatch(event)
File "/home/site/wwwroot/chevah/github_hooks_server/handler.py", line 52, in dispatch
return handler(event)
File "/home/site/wwwroot/chevah/github_hooks_server/handler.py", line 275, in issue_comment
self._setNeedsReview(
File "/home/site/wwwroot/chevah/github_hooks_server/handler.py", line 179, in _setNeedsReview
issue.pull_request().create_review_requests(
File "/home/site/wwwroot/.python_packages/lib/site-packages/github3/decorators.py", line 24, in auth_wrapper
return func(self, *args, **kwargs)
File "/home/site/wwwroot/.python_packages/lib/site-packages/github3/pulls.py", line 325, in create_review_requests
json = self._json(self._post(url, data=data), 201)
File "/home/site/wwwroot/.python_packages/lib/site-packages/github3/models.py", line 161, in _json
raise exceptions.error_for(response)
github3.exceptions.UnprocessableEntity: 422 Validation Failed

Same error here (after modifying PR description to not have reviewer: being empty).

danuker commented 2 years ago

needs-changes

danuker commented 2 years ago

needs-changes

danuker commented 2 years ago

needs-review (does not; just testing)

danuker commented 2 years ago

needs-changes

danuker commented 2 years ago

please review :)

Feel free to keep it in review for a week, while we test it extensively.

adiroiban commented 2 years ago

I think that the review request regex should be more relaxed... to allow requesting a review via this "natural language"

this is ready. Please review. Thanks!

I now found out about the CODEOWNERS feature. I think it can be used to automatically request a review from the team, when a PR is created.

But I think that our current implementation is better as you can re-request the review later. I have not used CODEOWNERS, but I guess it only ask for the team once. It might re-request is you mark a PR as draft and then back as final

adiroiban commented 2 years ago

CODEOWNER only adds the review first time the PR is created. it doens't re-add the team on re-review request. Even after marking a PR as draft and then as ready again.

So our hook is still very useful... unfortunately. The goal is to have all these actions supported by Github.

adiroiban commented 2 years ago

It would be nice if 'needs-review' would work for a normal review comment.

See example here https://github.com/twisted/twisted/pull/11629#pullrequestreview-1090797744

The idea is that you leave inline comments and then leave a final comment with the marker. And the review process is trigerred

adiroiban commented 2 years ago

Let me know when this is deployed.

We also need to allow please review. and please review! ... I guess

Thanks

danuker commented 2 years ago

We also need to allow please review. and please review! ... I guess

The regex I used does not match anything after please review.

So, your examples will match. But so will please review another PR not this one. Is this a problem?

adiroiban commented 2 years ago

So, your examples will match. But so will please review another PR not this one. Is this a problem?

It should not be a problem. This is what was asked by other Twisted devs.

danuker commented 2 years ago

needs-changes is still case sensitive. I didn't think it'd matter since we should be using reviews anyway.

danuker commented 2 years ago

But Please review! <-- should trigger the change.

I have deployed the last commit (#df0edf13).

Feel free to take more time again for testing before I merge.

adiroiban commented 2 years ago

This is already a branch too old and too big and already in prod.

I think is best to merge it.

If anything needs a fix we should create a separate PR.

Thanks!

adiroiban commented 2 years ago

we will need to see how to re-deploy it for Twisted Org only. Is ok to share it during development... but for production is best to have separate accounts.

btw... there a are now new personal API tokens with more granular access

adiroiban commented 2 years ago

https://github.blog/changelog/2022-10-18-introducing-fine-grained-personal-access-tokens/

danuker commented 2 years ago

we will need to see how to re-deploy it for Twisted Org only. Is ok to share it during development... but for production is best to have separate accounts.

All right. Should I create a new ProAtria Azure Functions app with the twisted-trac token? Or another, new user, maybe twisted-bot?

btw... there a are now new personal API tokens with more granular access

I saw them, but I can only create them for repos owned by my account.

adiroiban commented 2 years ago

I don't know what is best.

Twisted has its own Azure organization...so is best to host the functions there... but if you will be the one looking after the code, we can continue to keep it shared with our Chevah org.

I will try to find some time and see what can be done for Twisted.