Add PR edited event

[qwerty287]

Basically cherry-pick https://codeberg.org/crowci/crow/pulls/82 with some improvements.

@pat-s your implementation contains a security issue. It is possible to bypass the approval requirement because it does not check for the pr edited event if you require PRs to be approved. This is fixed here.

[woodpecker-bot]

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-5155.surge.sh

[anbraten]

Code lgtm quite fine to me already. Just two questions:
Will a user need to repair the repo to enable the new webhook event? (looks to me like we already receive them)
Will this event be triggered for label, milestone, assignee, ... updates as well?

[qwerty287]

Looks like we receive all of them for PRs already, but since they have separate hook type names, no, this doesn't include labels etc. Repairing is also not necessary.

[codecov]

Codecov Report

Attention: Patch coverage is 65.62500% with 11 lines in your changes missing coverage. Please review.

Project coverage is 26.38%. Comparing base (a02de81) to head (290b0c3).
Report is 72 commits behind head on main.

Files with missing lines	Patch %	Lines
pipeline/frontend/metadata/environment.go	0.00%	2 Missing ⚠️
server/forge/forgejo/parse.go	0.00%	1 Missing and 1 partial ⚠️
server/forge/gitea/parse.go	0.00%	1 Missing and 1 partial ⚠️
server/model/pipeline.go	0.00%	2 Missing ⚠️
server/api/hook.go	0.00%	0 Missing and 1 partial ⚠️
server/forge/github/parse.go	75.00%	0 Missing and 1 partial ⚠️
server/pipeline/create.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5155      +/-   ##
==========================================
+ Coverage   26.36%   26.38%   +0.01%     
==========================================
  Files         401      401              
  Lines       28537    28547      +10     
==========================================
+ Hits         7525     7533       +8     
- Misses      20323    20325       +2     
  Partials      689      689              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[6543]

I'm agains a pull edit event and in fafour of an pull/issue_metadata event that can trigger if e.g. lables, asignees etc changes

else we will get a longer and longer event enum list ...

[anbraten]

Perfect, so gitea / forgejo is already activated for those events. Github lgtm as well:

Did some testing with GitHub for other actions (label, milestone, draft / ready for review) and received those events by now (the part after the . is the action of the payload):

Edited seems to be only triggered by title and description updates as stated in the description. Changing draft state is another event for GitHub. At least compared to GitLab where the draft state is set by adding the draft prefix to a MR title this would be kind of inconsistent atm.

I would keep the implementation focused on the editing of title & description for now. We should however decide now if we want to add separate events like label added in the future as part of this pull_request_edited event or separate events and receive those as env vars like CI_PULL_REQUEST_LABELS=..., CI_PULL_REQUEST_DRAFT=, CI_PULL_REQUEST_MILESTONE=... etc. And add granular details what the user has to expect by that event to the docs.

linking #3425

[anbraten]

Option A

• use pull_request_edited just for changes to the pull-request title and description
• in the future we would add other events as sth like pull_request_labels, pull_request_milestone, ...

Option B

• fire event for all pull-request related metadata updates (title, description edits, label added / removed, milestone, draft / ready, assignee, reviewer) for now just title and description, but we add a hint to docs / ui that other parts will follow (should be pretty simple to add env vars for the other details later on)
• add the event as pull_request_edited (is this naming optimal as this event should probably be triggered for new pull-request and closed ones as well?)

I think I would prefer option b at as combining those events seems fine to me. Users would probably for example not need to filter secrets only for milestone events while allowing label change events.

Use-cases for me would be:

• check title to use conventail commits
• do some task when
  • a PR was marked ready-for-review after being in draft previously
  • a label was assigned (exp build the code)
  • a reviewer was assigned (update the reviewer in an external tool like jira)

[xoxys]

I would prefer Option A. The only reason for me to group them is the UX, but this can be improved. E.g. for secrets we could just provide grouping similar to the GH token scope:

Why should we prevent users who explicitly want to handle e.g. label or title edit events differently?

[qwerty287]

If we go with B, we need to extend the when filters to support all these different metadata change types. Otherwise it wouldn't be possible to differentiate between these as @xoxys said. Just going with evaluate seems always a bit improper to me, even if it is very powerful and extensible, but for simple and basic usecases I'd prefer a pure yaml solution.

If we go with A, we just should improve the dev experience by using a single package for constants and not having to redefine every event x times.

I don't really have a preference here.

E.g. for secrets we could just provide grouping similar to the GH token scope:

We don't do this at all currently. If the "pull request" event is enabled, the secret is available if the PR is opened, edited, closed, merged, updated, whatever. For ux and implementation reasons, I'd keep this behaviour. From a security perspective, splitting won't improve that as well as you can trigger any of these events when having access to the PRs in the forge.

[6543]

thanks for not rushing it, Im aware of option A and B ... well i see the pros and cons of both ...

I personally would go with B

as the events for title change, milestone additions etc is probably mostly used to handled by code checking for compliance of lables/titles etc...

just my usecases i would have:

• ensure a milestone is set
• ensure correct labels are set and where not altered

[6543]

anyway I'm looking forward to see labels-, milestones-, titles-changes trigger woodpecker automation :)

[6543]

-> feel free to remove my block once conclution has reached 🚀

[6543]

in #5214 as as alternative for this ... i try to implement the gitea/forgejo + docs part only and then we can decide if we wana have a meta event or single events for each ...

[6543]

@qwerty287 @anbraten was an dessision already made to go for #5155 or #5214 ?

[qwerty287]

I don't think so. There was no further discussion yet. I personally still would go with this one

[6543]

well my main argumen it is, that woodpecker should have all events forge independent ... while metadata on pulls can be different ineach forge, e.g. sone dont have projects etc ...

[pat-s]

while metadata on pulls can be different ineach forge

This doesn't matter for the discussion at hand. You can integrate any underlying forge event to be possibly included as a valid trigger for the WP event pull_request_edited as it is an abstract one.

One can discuss if users even want a build trigger on milestone or label change, or any other possibly related change to a PR, or if this should be restricted to the essential components, i.e. title and description. Arguments for both exists.

The major downside of squashing all actions into one will result in a lack of fine tuning for users. For example, in some workflows of mine I use commitlint to check the title. I would absolutely not want triggers on label changes. This would really be a bummer for me.
On the flipside, users would need to define more event triggers if they want triggers for all possible events. While this is a bit more work, I wonder if this is ever an "issue" and it this will ever apply in practice. I think its better to be specific here and avoid event triggers which are unspecific and leave one guessing which action caused a build to be triggered in the end.

In general, I don't understand what is wrong with this PR and why the initial work in #5214 has been put into. You could have just built upon this PR/extended it, propose a name change or wait for a resolution for the meta discussion. Instead now, there are two PRs and you're pushing hard to get the other one in, for hard to understand reasons (at least to me).
(Note that I don't care personally which decision is being made, Crow already has Option A and will also go with this narrow-event approach in the future).

In case a voting is happening, I'd vote for option A.

that woodpecker should have all events forge independent

I can't follow that argumentation as the current one is already forge-independent. However, your comment indicates it would not be?

[6543]

Thats why i want to just squash the meta events ... And introduce a New env var to fine Granulat filter in Thema if needed ... See my demo implementation

[6543]

You could have just built upon this PR/extended it, ...

Yes i used it as base, because i did want to have the test assets and if i would have done from scratch it would change/add similar lines so why not use it and by doing so also honor the init. work?

I would vote vor B and to not argue around non existing impl. i created my pull - this should not be about wicht author can merge his pull.
If B is choosen but my impl. is bad just propose a new one

One can discuss if users even want a build trigger on milestone or label change, or any other possibly related change to a PR

well if we want to cover what github-, gitea- or forgejo-actions can do ... we have too

[6543]

if we dont ... well then we should define a clear scope and stick to it

[confusedsushi]

I would prefer option B, for usability reasons.

The meta data on which this event might trigger could be very different for different forges. Bundle all meta data changes into a single event would allow Woodpecker to quickly handle new types of changes with that event. Documentation changes would be minimal if even needed. On the other hand having a single event for every type of change might does not scale well in the future.

As a user of a CI it is might hard to bundle different events into a single handler. But as a user of Woodpecker it would be quiet easy to split that single event into different handlers using evaluate. This might gets even more easy if evaluate learns more operators.

And still this would not prevent to handle some changes more special than others. But I would first like to have the generic one catches all approach.

[qwerty287]

Thinking over this again, I actually agree to this...
It is probably better to keep the event list clean and let users filter using evaluate. We don't even have to add separate "native " filters.
So I guess I'd also go with #5214.

[xoxys]

Ok looks like most people prefer this way, then Ill follow this opinion 👍

[xoxys]

Should we close this one then for now?

[6543]

will finish my pull and your work is not forgotten It's in my pull commit history :)