Skip to content

Add webhook payload size optimization options #35129

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Add webhook payload size optimization options #35129

wants to merge 11 commits into from

Conversation

kerwin612
Copy link
Member

This PR adds webhook payload size optimization options to help reduce payload size for large commits or pushes with many commits. This feature is particularly useful for target URLs that restrict payload size or have performance problems handling large JSON parsing.
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 19, 2025
@github-actions github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/migrations labels Jul 19, 2025
@wxiaoguang
Copy link
Contributor

It should be more flexible to use "limit number" instead of "bool"

@kerwin612 kerwin612 force-pushed the feature/webhook-payload-optimization branch 6 times, most recently from f249f67 to 47c395d Compare July 23, 2025 08:09
@kerwin612 kerwin612 force-pushed the feature/webhook-payload-optimization branch from 47c395d to 5472d66 Compare July 23, 2025 08:37
@kerwin612
Copy link
Member Author

kerwin612 commented Jul 23, 2025 •
edited
Loading

It should be more flexible to use "limit number" instead of "bool"

image Is this OK?

@kerwin612 kerwin612 requested a review from lunny July 30, 2025 04:39
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 30, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 30, 2025 •
edited
Loading

  1. IIRC all other places use "-1" for "no-limit" when "0" is used for "limit to zero".
    • For example: the limit of the repo number
  2. Maybe we need a JSON column but not many new separate columns.
  3. There are some issues asking about "commit order", so some users want the first N commits, while some other users want the last N commits

@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Jul 30, 2025
@kerwin612
Copy link
Member Author

image

@kerwin612 kerwin612 force-pushed the feature/webhook-payload-optimization branch from 5e828e9 to 258d8ee Compare August 4, 2025 07:07
@kerwin612 kerwin612 force-pushed the feature/webhook-payload-optimization branch 3 times, most recently from 81f8006 to f30ad48 Compare August 4, 2025 08:12
@kerwin612 kerwin612 force-pushed the feature/webhook-payload-optimization branch from f30ad48 to b718c54 Compare August 4, 2025 08:47
@lunny lunny added this to the 1.25.0 milestone Aug 5, 2025
@kerwin612 kerwin612 force-pushed the feature/webhook-payload-optimization branch from b1d8892 to 655bfcd Compare August 11, 2025 01:38
@kerwin612 kerwin612 requested a review from a team August 11, 2025 01:59
silverwind
silverwind reviewed Aug 11, 2025
View reviewed changes
services/forms/repo_form.go
@@ -622,7 +627,7 @@ type UpdateAllowEditsForm struct {
// | _// __ \| | _/ __ \\__ \ / ___// __ \
// | | \ ___/| |_\ ___/ / __ \_\___ \\ ___/
// |____|_ /\___ >____/\___ >____ /____ >\___ >
// \/ \/ \/ \/ \/ \/
// \/ \/ \/ \/ \/ \/
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated

silverwind
silverwind reviewed Aug 11, 2025
View reviewed changes
routers/api/v1/utils/hook.go
}
return defaultValue
}

Copy link
Member

Choose a reason for hiding this comment

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

These functions have oddly generic names but then retrieve hardcoded strings "enable" and "limit" from the map. They should be renamed to like getEnableInt and getLimitInt. Also I wonder if the nolint could be avoided.

silverwind
silverwind reviewed Aug 11, 2025
View reviewed changes
templates/repo/settings/webhook/settings.tmpl
field.disabled = !enabled;
}
}
</script>
Copy link
Member

@silverwind silverwind Aug 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Please move this js to web_src/js. We definitely don't want new inline scripts as they don't work with strict CSP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silverwind silverwind silverwind left review comments

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files modifies/translation
Projects
None yet
Milestone
1.25.0
Development

Successfully merging this pull request may close these issues.
5 participants
@kerwin612 @wxiaoguang @lunny @silverwind @GiteaBot