dorny / paths-filter

Conditionally run actions based on files modified by PR, feature branch or pushed commits
MIT License
2.13k stars 238 forks source link

Action sometimes fails on push #15

Closed GabriFila closed 4 years ago

GabriFila commented 4 years ago

Thanks a lot for this amazing action! We use it at CrownLabs@Polito

We had some action (1, 2) failing on pushes, this is the log (same on both):

Run dorny/paths-filter@v2.0.0
  with:
    filters: webservice:
    - 'webservice/**/*'

    token: ***
/usr/bin/git fetch --depth=1 origin 0000000000000000000000000000000000000000
fatal: remote error: upload-pack: not our ref 0000000000000000000000000000000000000000
fatal: the remote end hung up unexpectedly
##[error]The process '/usr/bin/git' failed with exit code 128

This is our action implementation. Can you tell us if we are doing something wrong

Thank you very much!

dorny commented 4 years ago

You're welcome :)

I can see the issue is here:

/usr/bin/git fetch --depth=1 origin 0000000000000000000000000000000000000000

Instead of those many zeroes there should have been SHA of the previous commit. SHA is taken from the before field of the push event payload.

There must be some reason why Github provided this invalid SHA. I can see your workflow is triggered on push but branches are not specified. Could this be the case for the first push of a new branch?

I will look into it.

GabriFila commented 4 years ago

Yes, you are right it always happens on the first push of the new branch. If you need other info from us, feel free to ask

chenlu-wang commented 4 years ago

I have the same error for first push of new branches. Is it ok to use ref field instead before field to get SHA? @dorny

dorny commented 4 years ago

To detect changed files we need a reference state (commit or branch) to which current state is compared to. For pull_request, the reference is obviously the target branch.

For push events, it could be either:

  1. The last previously pushed commit [current behavior] If there is no previously pushed commit then consider all files as changed [not implemented yet]

  2. Top of the "base" branch. Base branch would be the repository default branch but it could be overridden using action input parameter. [not implemented yet]

So the issue with the current implementation is that there are no previously pushed commits for the new branches.

@chenlu-wang using ref would not help here. We would compare the latest commit on the branch to the top of the branch (same commit), so no changed would be detected.

I will add support for "base" branch soon. It will basically make it work the same way as with opened pull request against the base branch, but without the PR itself.

Would this work for your use case?

chenlu-wang commented 4 years ago

@dorny Thanks for your explanation, it sounds sensible to compare to base branch as a reference state when there is no previous push commit, which should fix the error in our case.

The changes will dynamically depends on what is it on base branch. It maybe better to consider all the files as changed -- as there is nothing on previous commit.

GabriFila commented 4 years ago

@dorny thanks for the explanation.

The behavior

Top of the "base" branch. Base branch would be the repository default branch but it could be overridden using action input parameter. [not implemented yet]

would be great for us!

Shatur commented 4 years ago

Top of the "base" branch. Base branch would be the repository default branch but it could be overridden using action input parameter. [not implemented yet]

Same here. Would be nice to have. If check diffs between commits, then the next commit may not cover all the tests of the previous one, but CI will write that everything went well, which will be very dangerous.

dorny commented 4 years ago

You can update to v2.2.0. New default behavior is: master branch - changes are detected against the most recent commit before the push feature branches - changes are detected against the repository default branch, usually master.

You can change the default behavior by setting the base input parameter. See the README for more information.

If this solution doesn't solve your needs, feel free to reopen this issue or create a new one.

GabriFila commented 4 years ago

This is great Thank you very much!