RFC: How to add new pipeline events without needing to ignore them each time

[anbraten]

We may add new pipeline events in the future:

Possible events:

• PR close Support PR closed event #286
• Release (type: draft, pre, stable; action: created, updated, deleted) Add support for release event (Github, Gitea, Gitlab)  #764
• Cron Cron triggers #8 Add cron jobs #934
• Support manually triggering a pipeline Add support to trigger pipelines manually #83 Create build from API? #240
• comments (create, delete, update) / Add support for comment commands #814
• labels (added, removed, issue, PR)
• issues (created, updated, closed, reopened)

Adding a when.event filter for each pipeline or step would be quite complex and having a default value like skipping crons should probably be avoided as it can not easily be expected by an user:

Maybe we can add a new on / trigger keyword instead. By default a pipeline would not be executed at all. The linter could throw an error if no "trigger" is set at all. If a user adds one or multiple "trigger" events the pipeline will be executed for those events and contained steps can be filtered by when.event.

originally from #286 (comment)

[gapodo]

A global triggered_by or on taking events could make sense, else I'd go with "if it's not filtered, it's a free for all".

Either explicitly specify or take all current and future events. Basically, when using wildcards or not filtering at all, I'd expect that as the default behavior THB.

Also, since some discussions on the Pipeline structure are still open (IIRC), are we currently completely non-breaking or should the new format get a version attribute anyway, which would then let us handle events based on config version (I believe once new keywords are added versioning will somewhat need to enter the picture anyway).

[anbraten]

Either explicitly specify or take all current and future events. Basically, when using wildcards or not filtering at all, I'd expect that as the default behavior THB.

I am totally with you on that point. Already got kicked by that default filter for crons a few times 🙈

We are trying to keep the config changes as minimal as possible, but if it is a needed change we would break things before releasing 1.0. That's why we think and try to get all possible breaking changes #869 (comment) done before releasing 1.0 so we do not break again and again.

There was the idea to have a completely separated code unit for the config compiling which would allow to add config version, but honestly that's pretty complicated and would probably require quite some effort to support old compilers in the long term.

[gapodo]

There was the idea to have a completely separated code unit for the config compiling which would allow to add config version, but honestly that's pretty complicated and would probably require quite some effort to support old compilers in the long term.

Jep, that would be some effort, what I would take into consideration would be to have the version in the file at least from 1.0 upwards, so if there ever arises a need, it would be easy to differentiate. Also having versions would allow showing deprecation notices (failing a build until deprecation_know is set to true?), so that a switchover would only mean maintaining 2 compilers/versions for a limited amount of time or potentially handle the changes longer if they are easy to maintain.

An example would be this issues base idea, if we don't find a on, trigger triggered_by or what ever in the definition assume it's old and only trigger on the old events.

[smainz]

Maybe I do not get it, but what would be the difference between (on a pipeline / workflow level):

when:
  - event:
    - push
    - pull_request
  
pipeline:
  - name: First step
 
...

and

on:
  - push
  - pull_request
 
pipeline:
  - name: First step

If possible I would not introduce new keywords for already existing concepts. You have twice the time to document it and users need to learn twice.

In the end (#745, #567) It could be something like this:

pipeline:
  when:
    - event: 
      - push
      - pull_request
    - cron
     
  workflows:
    - name: Build pipeline
      when:
        - push
        - branch: master
      steps:
       - name: Prepare build
         image: alpine
         commands:
           ...

    - name: PR pipeline
      when:
        - event: pull_request
      steps:
        - name: Prepare PR
          image: alpine

The further down a condition is, the smaller the scope.

[anbraten]

Maybe I do not get it, but what would be the difference between (on a pipeline / workflow level):

Good point. Honestly there wouldn't be a huge difference. The main change I had in mind is forcing the user to set a top level filter for events. But now I think maybe removing the default filter would be enough, so the user would simply get a lot of pipelines in case he misses to set a filter as that's easily noticeable and fixable.

As we always have one workflow per file (and I think there is no plan to change it) you can filter per workflow (top-level) and per step, so there wouldn't be a need to change a lot I guess.

[gapodo]

so the user would simply get a lot of pipelines in case he misses to set a filter

It might be a good idea to add a config flag allowing to prevent woodpecker from running without having top level event filters set (even if a user sets it to *), this would be especially useful when talking about shared environments (like Codeberg-CI) where a version upgrade happens centrally and could cause a huge waste of ci-time until everybody fixed it (if they even care about it, which may not always be the case).

[smainz]

A config to force users to add some when: ... condition might be a way to force a migration to the new way, but do we really need to drop the current filter?

Do nothing -> everything stays the same. If one wants to have more events: add a when condition.

[gapodo]

Do nothing -> everything stays the same. If one wants to have more events: add a when condition.

IMHO that's a bad idea, it would be keeping some legacy behavior as default (I'd leave it for legacy configs, if we would really implement config-schema versions and handle them differently), which then is going to confuse users joining in after 1.0, also it would basically mean some events are opt-in and some opt-out, which is fairly opaque when working with a system and mostly only makes sense when you know the legacy behind it (which is the effective reason for that behavior).

Also, the 'If one wants to have more events: add a when condition.' would mean you'll need to list all events in the when.event to default to all events instead of just not setting a when.event.

[smainz]

Do nothing -> everything stays the same. If one wants to have more events: add a when condition.

IMHO that's a bad idea, it would be keeping some legacy behavior as default (I'd leave it for legacy configs, if we would really implement config-schema versions and handle them differently), which then is going to confuse users joining in after 1.0, also it would basically mean some events are opt-in and some opt-out, which is fairly opaque when working with a system and mostly only makes sense when you know the legacy behind it (which is the effective reason for that behavior).

Ok, I can agree on that one.

Also, the 'If one wants to have more events: add a when condition.' would mean you'll need to list all events in the when.event to default to all events instead of just not setting a when.event.

In my experience, you never want all events, but you will not want no events too. So I am undecided on this one.

[gapodo]

In my experience, you never want all events, but you will not want no events too. So I am undecided on this one.

Then we'll need to decide on "all or nothing" either you have to always specify a filter (and it's only triggered if it's matching) or we default to all events and users will have to filter out what they don't want. Both have their merits, using a default of no match requires the user to specify exactly what he wants (and therefore should be "non-breaking" if we add new events), defaulting to all means a user can either filter or "just not care" (which is what I believe a good pipeline should be able to do, basically handle all cases or fail / abort if it's not needed).

The advantage of defaulting to none (explicitly declare on which events) only holds true, if we disallow wildcards and exclude (else the pipeline will potentially take up new events added later on).

And I'm tempted to say mixing defaults (some default on others off) would be the worst option as it would mess with consistency in docs, the mindset,...

[smainz]

I think, we should go with all, because giving up on wildcards or excludes is too much of a sacrifice.
A normal pipeline should be non-destructive, so executing it too many should not harm. My deployment piplines e.g. always filter for the branch (e.g. main) and will not push anything into production even if a new event is introduced.

Docs should be clear on that though.

[6543]

Whell what if we have a ... well we did once got rid of it ... filter for events in the repo config with by default only the "tag, pull, push, deploy" events allowed? so we have a default filter but its per repo settings

?

[qwerty287]

To solve this issue, my suggestion is to add a recommendation/warning to the linter.

If a pipeline step has no event filters and thus would have been executed on every one, warn the users about this behavior. It should be a best-practice to define event filters I think, so we can also lint for it.

[anbraten]

That sounds like a good idea.