-
Notifications
You must be signed in to change notification settings - Fork 40
fix: Support custom repository_dispatch actions #712
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
fix: Support custom repository_dispatch actions #712
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
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.
Pull Request Overview
This PR adds support for user-defined repository_dispatch actions by introducing a custom event type and updating the converter to fall back to it when an action isn’t recognized.
- Added a new
RepositoryDispatchCustomEventrecord inheriting fromRepositoryDispatchEvent - Updated
WebhookConverterto detect unregistered actions and route them to the custom event - Provided a sample JSON payload for testing custom
repository_dispatchevents
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/Octokit.Webhooks.Test/Resources/repository_dispatch/custom.payload.json | New sample payload illustrating a custom action |
| src/Octokit.Webhooks/Events/RepositoryDispatch/RepositoryDispatchCustomEvent.cs | New empty record type for custom repository_dispatch events |
| src/Octokit.Webhooks/Converter/WebhookConverter.cs | Logic updated to map unknown actions to the custom event type |
Comments suppressed due to low confidence (3)
src/Octokit.Webhooks/Converter/WebhookConverter.cs:36
- [nitpick] Using a local variable named
typeshadows theSystem.Typeclass and can cause confusion. Consider renaming this variable toeventTypeortargetTypefor clarity.
Type? type = null;
test/Octokit.Webhooks.Test/Resources/repository_dispatch/custom.payload.json:1
- A JSON sample for a custom
repository_dispatchevent has been added, but there isn’t a corresponding unit test to deserialize it intoRepositoryDispatchCustomEvent. Please add a test inWebhookConverterTeststo verify this mapping.
{
src/Octokit.Webhooks/Converter/WebhookConverter.cs:46
- The fallback condition for custom repository_dispatch events checks if typeToConvert == typeof(RepositoryDispatchEvent), but the converter is registered for WebhookEvent so this never matches. You may need to detect repository_dispatch by inspecting the JSON or adjust the type check to ensure custom events are correctly routed.
if (type == null && typeToConvert == typeof(Events.RepositoryDispatchEvent))
- `dynamic` is a `JsonElement`. - Add integration tests. - Pick up fix from octokit/webhooks.net#712.
- `dynamic` is a `JsonElement`. - Add integration tests. - Pick up fix from octokit/webhooks.net#712.
- Add support for deserializing user-defined `repository_dispatch` payloads. - Simplify reflection to get attributes. - Use dictionary instead of iterating it. - Use constant instead of computing string per-deserialization.
44f0a20 to
9251df1
Compare
|
Thanks for this! But since I merged #708, this is going to come in a major release. |
Do you have a rough ETA on v3? Would prefer to not have to consume the NuGet packages from CI directly (I don't like the need to use a GitHub PAT to get packages from the repo's NuGet artifacts feed). |
|
@martincostello I need to get #716 in, and I am also considering #710. I was wondering if I could get your opinion on it please? |
|
@martincostello did v3.0.0 work for you? |
|
Yes, great thanks! |
Resolves #711
Before the change?
repository_dispatchevents cannot be deserialized.After the change?
repository_dispatchpayloads can be deserializedPull request checklist
Does this introduce a breaking change?