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

v3 on: [pull_request] doesn't actually check out branch, it rather checks out a version merged/based on top of the target branch, including changes from the target branch #881

Open
thisismydesign opened this issue Aug 5, 2022 · 11 comments

Comments

@thisismydesign
Copy link

Similar/same as
#237
#299
#626
#504

This issue isn't resolved on v3. It's very unexpected that instead of checking out the actual code I'm working on in the PR, the CI runs on a completely different code. I.e. code from the target branch will be included in the checked-out code. Can this even produce consistent behavior? What happens in case of conflict with the target branch?

GitHub has a feature to enforce keeping PRs up to date for those who wish to do so

image

I opened a new issue because the others were either closed as fixed, or the titles were not descriptive and there were no comments from maintainers. Can we explain whether this is even expected behavior?

@thisismydesign thisismydesign changed the title v3 on: [pull_request] doesn't actually check out branch, it rather checks out a version merged/based on top of the target branch v3 on: [pull_request] doesn't actually check out branch, it rather checks out a version merged/based on top of the target branch, including changes from the target branch Aug 5, 2022
@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Aug 17, 2022

#112 added documentation on how to avoid the merge: https://github.com/actions/checkout/blob/v3.0.2/README.md#user-content-checkout-pull-request-head-commit-instead-of-merge-commit

@thisismydesign
Copy link
Author

Thanks, so it's expected behavior. In that case, I'm asking to change it.

What happens in case of conflict with the target branch?

Meanwhile, I discovered that the CI would not run in this case. This is also a big drawback. Firstly it's unexpected and takes time to figure out why. But more importantly, I rely on the CI to run. E.g. I might run some checks (e.g. unit test) locally but would expect the CI to tell me if my changes integrate well with other parts of the software. Now I have to resolve conflicts every time as soon as they occur so that I can continue development? Awful.

@KalleOlaviNiemitalo
Copy link

I'm curious, how did you start using the checkout action in your workflow; did you copy the workflow from a template? If so, perhaps the template could be improved by adding some comments to explain the behaviour.

@thisismydesign
Copy link
Author

@KalleOlaviNiemitalo Found it in a project I started to work on :)

There are many reasons to use on: pull_request trigger (instead of on: push), e.g.

  • Will trigger on contributions from forks, without the contributor having to enable actions on their fork (and potentially use their own CI minutes)
  • You can push WIP state without triggering the CI
  • Run things only on feature branches without the need to explicitly restrict branches

But that's beside the point I think. The point is that this is very surprising (as evidenced by the issues linked above) and therefore this probably isn't the right way to go about it. https://en.wikipedia.org/wiki/Principle_of_least_astonishment

@mattsh247
Copy link

mattsh247 commented Aug 18, 2022 via email

@sidekick-eimantas
Copy link

sidekick-eimantas commented Aug 24, 2022

I'm trying to get the list of files modified since main branch on pull request, in python code, and because of the checkout behaviour of this action I've now been stuck on the problem for like 4 hours.
fatal: Not a valid object name main
fatal: ambiguous argument 'main': unknown revision or path not in the working tree.
etc..

The solution @KalleOlaviNiemitalo mentions above does not work.

@KalleOlaviNiemitalo
Copy link

To clarify: I got here from dotnet/Nerdbank.GitVersioning#796 (comment) and am not using this action myself.

@dlupu
Copy link

dlupu commented Aug 29, 2022

Having similar issues on a private project, the checked-out code in the github action is not the code of the commit that has triggered the CI.

Same use case (v3 on: [pull_request] )

@nahojs
Copy link

nahojs commented Nov 17, 2022

I had similar issues but came up with this solution after some extensive digging. If checking out code using actions/checkout@v3 and trigger is pull_request the ref parameter needs to be github.event.pull_request.head.ref (for push use github.ref) Depth is default 1 (shallow clone), so try different values. 0 is full history. Complete example:

env:
  MY_REF: ${{ format('refs/heads/{0}', github.event.pull_request.head.ref) }}

[..]
     - name: Checkout Requests
          uses: actions/checkout@v3
          with:
            repository: My.Repo
            ref: ${{ env.MY_REF}}
            fetch-depth: 0

define-null added a commit to electric-sql/electric that referenced this issue Nov 28, 2022
Fix: github CI to use actual branch commit instead the merge on
top of main. (actions/checkout#881)

Fix: caching for docker and CI to make it more strict. We no longer
pick local-build image on CI for integration tests

Fix: migration tests that previously were skipped on CI

Fix: postgres manager is handling multiple requests originated from
the schema.

Fix: satellite protocol code would previously crash on start/stop/start
replication flow.
define-null added a commit to electric-sql/electric that referenced this issue Nov 28, 2022
Fix: github CI to use actual branch commit instead the merge on
top of main. (actions/checkout#881)

Fix: caching for docker and CI to make it more strict. We no longer
pick local-build image on CI for integration tests

Fix: migration tests that previously were skipped on CI

Fix: postgres manager is handling multiple requests originated from
the schema.

Fix: satellite protocol code would previously crash on start/stop/start
replication flow.
define-null pushed a commit to electric-sql/electric that referenced this issue Nov 28, 2022
Fix: github CI to use actual branch commit instead the merge on
top of main. (actions/checkout#881)

Fix: caching for docker and CI to make it more strict. We no longer
pick local-build image on CI for integration tests

Fix: migration tests that previously were skipped on CI

Fix: postgres manager is handling multiple requests originated from
the schema.

Fix: satellite protocol code would previously crash on start/stop/start
replication flow.
ddadaal added a commit to PKUHPC/OpenSCOW that referenced this issue Feb 28, 2023
在启动auth, portal-web, mis-web,
portal-server和mis-server时在日志中打印版本信息,包含`tag`和`commit`。npm会打印package.json的信息


![image](https://user-images.githubusercontent.com/8363856/221754574-5b9b467a-aed4-438f-bdb1-7811636ae563.png)


注意:PR构建的容器的所log出来的commit号不是PR的commit的commit号!参考:actions/checkout#881

实现:
1. 在构建容器时,根据构建容器时的最新commit,生成一个`version.json`文件放在容器pwd下
2. 在这些组件启动时读取version.json和package.json的内容并生成内容
@janderssonse
Copy link

janderssonse commented Apr 14, 2023

If there are technical issues fine, but in all other aspects (as issues tell), the expected behaviour for a user would be as the @thisismydesign described it, (the different users reporting issues about it this tells the same story). As an user I would expect it to checkout the latest code on a pr - i also had to do a workaround with refs explicitly. Anyway, thanks for the action!!

devonwebb1088 added a commit to devonwebb1088/elixir-electric-app that referenced this issue May 21, 2023
Fix: github CI to use actual branch commit instead the merge on
top of main. (actions/checkout#881)

Fix: caching for docker and CI to make it more strict. We no longer
pick local-build image on CI for integration tests

Fix: migration tests that previously were skipped on CI

Fix: postgres manager is handling multiple requests originated from
the schema.

Fix: satellite protocol code would previously crash on start/stop/start
replication flow.
@giuliano-macedo
Copy link

giuliano-macedo commented Oct 28, 2023

I think the issue is with github actions itself and not with the checkout action, since the checkout action uses by default the equivalent of {{github.sha}} which is the default branch for that trigger, and so for pull request trigger it does this merging before hand and results in this weird behavior.

toofar added a commit to daerich/qutebrowser that referenced this issue Dec 2, 2023
This is just to get the CI to re-trigger.

Apparently the checkout action doesn't actually checkout the branch
being proposed for merge but checks out an actual merge commit. So if
you push a PR while main is broken it'll say changes from main are on
your branch. That's a little unexpected.

main is fixed now and I tried re-running the CI jobs from the web UI but
they are still failing with the same errors. Hence this merge of main
just to get a change on the branch. I could have gone and found a typo
to fix instead. I know I've left enough of them around.

ref: actions/checkout#881
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants