Change value of "CI_COMMIT_BRANCH" for pulls to source branch

woodpecker-ci / woodpecker

Woodpecker is a simple, yet powerful CI/CD engine with great extensibility.

https://woodpecker-ci.org

Apache License 2.0

4.07k stars 351 forks source link

Change value of "CI_COMMIT_BRANCH" for pulls to source branch #3560

Open wxiaoguang opened 5 months ago

wxiaoguang commented 5 months ago

Describe the bug

Somewhat related to " PR triggers the CI twice #2888 ".

To reproduce:

Create a simple yaml in main branch:

steps:
  test-when:
    when: 
      branch: main
    image: ....
    commands: 
      - echo "running on CI_COMMIT_BRANCH=$CI_COMMIT_BRANCH"

Create a new branch new-branch based on main and open a PR on Gitea
Make some changes in new-branch branch
When woodpecker receives the pull_request_sync event for the PR branch new-branch, it still runs it and considers it as main branch
Expected behavior: such event should be skipped

System Info

server & client: 2.4.1

Additional context

No response

Validations

[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Checked that the bug isn't fixed in the next version already [https://woodpecker-ci.org/faq#which-version-of-woodpecker-should-i-use]

qwerty287 commented 5 months ago

If I understand you correctly that's intended. For PRs, the branch is the PR target branch.

From docs about the env var:

commit branch (equals target branch for pull requests)

wxiaoguang commented 5 months ago

But it is quite counter-intuitive that a non-main branch runs with when: branch: main.

I think the filter should be respected.

Expected behavior: such event should be skipped

qwerty287 commented 5 months ago

But it is quite counter-intuitive that a non-main branch runs with when: branch: main.

And what's the branch if it's a PR from a fork?

This is also described in the docs: https://woodpecker-ci.org/docs/usage/workflow-syntax#branch

The step now triggers on main branch, but also if the target branch of a pull request is main. Add an event condition to limit it further to pushes on main only.

wxiaoguang commented 5 months ago

Thank you, but it is just counter-intuitive at first glance ....

anbraten commented 5 months ago

Might actually make a bit more sense to have branch = source branch instead of target branch. That would however be a breaking change we could only consider for 3.0

wxiaoguang commented 5 months ago

Should I reopen this issue?

anbraten commented 5 months ago

Sure if that's a thing you would like to be changed.

wxiaoguang commented 5 months ago

I can see there could also be breaking changes in 2.x, eg: Use map on all environment keys in our config https://github.com/woodpecker-ci/woodpecker/pull/3500#issuecomment-2024396711

So is it possible to take this breaking change in 2.5.x ?

anbraten commented 5 months ago

How, did I removed that. 🤔 Sorry @qwerty287 that wasn't on purpose.

The comment was basically:

I can see there could also be breaking changes in 2.x, eg: Use map on all environment keys in our config https://github.com/woodpecker-ci/woodpecker/pull/3500#issuecomment-2024396711

This should not be a breaking change and might be a bug, if so we should open a new issue.

So is it possible to take this breaking change in 2.5.x ?

As it is a breaking change, we can only change that in version 3.0

wxiaoguang commented 5 months ago

I can see there could also be breaking changes in 2.x, eg: Use map on all environment keys in our config #3500 (comment)

This should not be a breaking change and might be a bug, if so we should open a new issue.

OK, I managed to figure it out .... I guess it is the changed behavior of handling dots . in environment names (no idea what it is related to, maybe related to docker)

For example: ES 7 documents use "environment: node.name=..." https://www.elastic.co/guide/en/elasticsearch/reference/7.17/docker.html . These environments don't work for the latest.