Change value of "CI_COMMIT_BRANCH" for pulls to source branch

[wxiaoguang]

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

• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• 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]

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]

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]

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]

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

[anbraten]

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]

Should I reopen this issue?

[anbraten]

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

[wxiaoguang]

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

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

[anbraten]

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 #3500 (comment)

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]

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.