-
Notifications
You must be signed in to change notification settings - Fork 729
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 team, team_add, member and membership payloads #1818
Conversation
@bitwiseman this is the PR I was talking about in another thread. I would appreciate if we could shoe it in the new version that you planned to release soon. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1818 +/- ##
============================================
+ Coverage 80.66% 80.69% +0.02%
- Complexity 2342 2350 +8
============================================
Files 220 223 +3
Lines 7082 7173 +91
Branches 379 393 +14
============================================
+ Hits 5713 5788 +75
- Misses 1132 1139 +7
- Partials 237 246 +9 ☔ View full report in Codecov by Sentry. |
I checked the codecov report and I think the test coverage is as good as it can be. I'm having a look at the Java 17 site issue as I was running things with Java 11 and apparently, it's more strict with Java 17. |
I removed/updated some outdated files that were apparently added in the hope of someone implementing the payloads but I preferred to start fresh with new updated payloads. I did a few adjustments to some existing enums to make sure we don't have an error when a payload arrives with an unknown value. I used the same pattern we decided to implement some time ago.
CI is all green now. |
I prepared the integration in Quarkus GitHub App here: quarkiverse/quarkus-github-app#584 . |
private FromToPermission permission; | ||
|
||
/** | ||
* Get changes to permission. | ||
* | ||
* @return changes to permission | ||
*/ | ||
public FromToPermission getPermission() { | ||
return permission; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsmet
Ugh. The events api is all over the place. The added
action has a role_name
and states that permission
is obsolete. But the edited
action has permission
and old_permission
, with no note about either of those being obsolete. And they both are returned under the changes
parameter.
https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=added#member
https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=edited#member
I guess this class is okay. It is incomplete, but the other fields can be added later without causing a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some tests for permission and old_permission wasn’t there.
I need to check for role_name though. I will have a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah so that's what I thought, role_name
wasn't in the payload I obtained...
They might change it at some point, I don't know. I'll have a look at adding role_name
for a future version but it's going to be untested.
Description
I removed/updated some outdated files that were apparently added in the hope of someone implementing the payloads but I preferred to start fresh with new updated payloads.
I did a few adjustments to some existing enums to make sure we don't have an error when a payload arrives with an unknown value. I used the same pattern we decided to implement some time ago.
See https://docs.github.com/en/webhooks/webhook-events-and-payloads .
Before submitting a PR:
@link
JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .mvn -D enable-ci clean install site
locally. If this command doesn't succeed, your change will not pass CI.main
. You will create your PR from that branch.When creating a PR: