actions / checkout

Action for checking out a repo
https://github.com/features/actions
MIT License
5.85k stars 1.73k forks source link

Checkout does not fetch the latest target branch when workflow re-run #1036

Closed ddongminstar closed 1 year ago

ddongminstar commented 1 year ago

In case of on pull request event,

With default settings,

use: actions/checkout@v3

As input option reference is empty, reference and SHA will be filled up with github context.

In case of workflow re-run, github context is same with initial workflow run context - https://docs.github.com/en/actions/managing-workflow-runs/re-running-workflows-and-jobs

If commit id is not empty we use commit-id to make refSpec and fetch the specific merge reference's commit. - https://github.com/actions/checkout/blob/main/src/ref-helper.ts#L93

So workflow checkout act as deterministic and does not match with target branch's latest commit if changed.

But if we made any new commit to PR, it will work as expected. (As commit id changed, fetch the merged version of latest target branch)

It works like checkout workflow cache the result internally as https://github.com/actions/checkout/issues/696 and https://github.com/actions/checkout/issues/461 mentioned.


We use github actions to CI and many tester and verifier.

In case of default branch's test code failed, we made a new pull request and merge to fix test failure. But We have to tell all other PR's author to made a any new commit or rebase and force push to pass the test.

So we made little trick like

use: actions/checkout@v3
with:
  ref: ${{ github.ref }}

just as same with actual reference

Checkout workflow run as non-deterministic but good for deal with target-branch's test failure.


So I think it will be great if there will be some document like checkout will not follow the target branch's latest commit in case of workflow re-run and you can make it follow by putting same reference as option.

Or to make it more understandable, how about provide a new option to choose whether use commit or use reference when fetching

I'm not sure this is a big issue, but please consider some document or options. Thank you in advance.

marc-hb commented 1 year ago

I think it's better like it is now. Sometimes you want to re-test the very same merge commit, for instance when there was a network issue. Other times you want to re test against the latest target branch. Right now you can do both:

If the "re-run" button does the latter then how do you do the former?

In any case it should be better documented, maybe here: https://github.com/actions/checkout#Checkout-pull-request-HEAD-commit-instead-of-merge-commit

vbuberen commented 1 year ago

Just found out this issue as well while was trying to understand why changes are not checked out by a workflow. In case logs are needed here it is where it is clearly visible that action defaults to main branch after re-run: https://github.com/fluttercommunity/plus_plugins/actions/runs/3670853327/jobs/6205663810

Agree with the reporter that documenting such case better would be beneficial.

vanZeben commented 1 year ago

To start, I want to say thanks a lot for providing some verbose feedback here. It's really good to see the community engage with us and be very clear with their problems. So thank you :) Secondly, I want to clarify on this a bit, as I think there is some contextual information about Pull requests that is missing and causing the confusion here.


In case of on pull request event,

With default settings,

use: actions/checkout@v3

...

From what I gather with the initial issue, you're looking for a way to keep Pull Requests up to date with the inclusion of more recent commits from the target branch's history using the pull_request event. Would that be correct?

If so, this isn't necessarily something that's intended directly by actions/checkout rather it's intended by the nature of the PR's themselves and how workflows are triggered off those PR's with the pull_request event. At a high level, your branch (which is associated to the PR) is forked off of a specific commit from the main branch and works in isolation from that point forward. When we run workflows with the pull_request event, they are contextualized to the Pull request itself, and so any updates to the target, aren't visible by the workflow until the Pull request is updated to match and as @marc-hb noted, there are ways to update PR's to test workflows with latest changes and we definitely want to preserve the deterministic nature of workflows with their context.

Something that might be helpful to you is that we do also have some documentation about keeping PR's in-sync. What this will do is help to alert authors of PR's automatically when there are changes to the target branch and prevent merging on their PR's if they don't have the latest changes (specifically take a look at this section in our docs if you want to make PR's more strict in this sense).


In regard to @vbuberen with;

Just found out this issue as well while was trying to understand why changes are not checked out by a workflow. In case logs are needed here it is where it is clearly visible that action defaults to main branch after re-run: https://github.com/fluttercommunity/plus_plugins/actions/runs/3670853327/jobs/6205663810

Agree with the reporter that documenting such case better would be beneficial.

I took a look into your case and I think it differs from the above, and I wouldn't classify this as the same issue originally noted, although contextually it has the same core misconceptions about the purpose of actions/checkout. Your workflow uses the event trigger pull_request_target, which to quote directly from the docs on the usage of this (which you can find here)

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does.. {...} Avoid using this event if you need to build or run code from the pull request.

What that means in the context of this actions and workflows using this action, you are the target branch and the the changes in the pull request that triggered the event are not included, which is why you need to use a workaround "re-center" the ref of the PR.

I'm not sure the specifics of your workflows, however unless you need to write permission to the target repository, or access to the target's secrets, I would recommend avoiding using pull_request_target except for cases where you need that write permission. We have a pretty lengthy security post about the dangers of pull_request_target that might be helpful to read, which (if you look at the code snippet for insecure pull_request_target cases in that document) actually includes retargeting the actions/checkout with the HEAD ref SHA in the same same manner as you did.


Given that both these comments aren't directly related to actions/checkout and from my perspective more are about how pull requests work or specific workflow event's work and we have other documentation directly talking about issues, Im going to close this.

If I've wildly misunderstood your problems, please reopen this issue and tag me directly and I would be happy to address your concerns :)

marc-hb commented 1 year ago

https://en.wikipedia.org/wiki/A_picture_is_worth_a_thousand_words

Git graphs made this truer than ever.

I generally avoid branching discussions that use only words, they're at best very time consuming and at worst a waste of time because people talk across each other.

Unlike "plain git", many github projects tend to have a linear history. This gives users the wrong impression that they can discuss git issues without any graphical representation. Big mistake.

None of these pages mentioned above has any git graph on them:

I haven't read every single docs.github.com page (obviously not) but I've read many over the years and I don't remember seeing a git graph in any of them. This is a huge documentation mistake. Searching the internet for something like "git merge strategy" shows thousands of pictures of git graphs - and rightly so.