Skip to content
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

Setting for empty commits on path condition #3708

Merged
merged 6 commits into from
May 14, 2024

Conversation

da-Kai
Copy link
Contributor

@da-Kai da-Kai commented May 14, 2024

Allow to set the behavior on empty commits evaluating path conditions.

default value is true, to preserve current behavior.

@6543 6543 added the pipeline-config related to pipeline config files (.yaml) label May 14, 2024
@qwerty287 qwerty287 added the enhancement improve existing features label May 14, 2024
@qwerty287
Copy link
Contributor

Hello and thanks for your contribution!
In general, the code looks good, and you also added tests and updated the schema which is something we often forget ;)

If I get the code correctly, settings on_empty: true will run the step (except other conditions are false) and on_empty: false will not run it?
Can you somehow make this more clear in the docs? Because tbh I don't really get what happens there just from the name. on_empty: true can mean "apply path filters if empty", but it can also mean "run step if empty".

And can you clarify in the docs that this will also apply to events that will never have a path? E.g. tags and releases.

And could you add that to one branch in https://github.com/woodpecker-ci/woodpecker/blob/main/pipeline/frontend/yaml/linter/schema/.woodpecker/test-when.yaml so we test the schema?

@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented May 14, 2024

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

@da-Kai
Copy link
Contributor Author

da-Kai commented May 14, 2024

Hello @qwerty287,

I expanded the documentation a bit. I hope it is now a little clearer. 😊

I added on_empty to when-path-include-exclude in
pipeline/frontend/yaml/linter/schema/.woodpecker/test-when.yaml

Co-authored-by: qwerty287 <80460567+qwerty287@users.noreply.github.com>
@qwerty287 qwerty287 merged commit faf6b33 into woodpecker-ci:main May 14, 2024
6 of 7 checks passed
@woodpecker-bot woodpecker-bot mentioned this pull request May 14, 2024
1 task
@anbraten
Copy link
Member

If I get the code correctly, settings on_empty: true will run the step (except other conditions are false) and on_empty: false will not run it?
Can you somehow make this more clear in the docs?

@qwerty287 As both of use were not sure about this option when looking at it for the first time, might it be a good idea to call it sth like ignore_when_no_file_changed: true instead?

@da-Kai What do you think?

@qwerty287
Copy link
Contributor

In general, I'd be fine with another name, but ignore_when_no_file_changed is too long for a yaml config key I think.
But I'd also say if documentation is clear enough it's fine to have some unclear points in the config. Yes, it's nice to read everything from yaml, but if something is not 100% clear you still have the docs.

@da-Kai
Copy link
Contributor Author

da-Kai commented May 16, 2024

@anbraten @qwerty287
I agree that on_empty is not very clear. But I also didn't want to use a name that was too long.

Maybe one of these 2?

  • pass_on_empty_commit
  • ignore_empty_commit

@qwerty287
Copy link
Contributor

I wrote that multiple times already: the term "empty commit" is just wrong in this context. There are events that are not commits but are "empty" (so don't have files) too, for example tag and release.

That's why it's not that easy to find a good name here... Using something like on_empty_paths is some kind of a duplicate because the property is part of the path object already so actually it states "if path is empty" already.

@da-Kai da-Kai deleted the path-condition-on-empty branch June 7, 2024 11:49
6543 pushed a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improve existing features pipeline-config related to pipeline config files (.yaml)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants