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

RFC: fetch-depth: 1 and not cloning tags are dangerous defaults #217

Closed
kiprasmel opened this issue Apr 15, 2020 · 17 comments
Closed

RFC: fetch-depth: 1 and not cloning tags are dangerous defaults #217

kiprasmel opened this issue Apr 15, 2020 · 17 comments

Comments

@kiprasmel
Copy link

kiprasmel commented Apr 15, 2020

I've had huge problems with github actions, specifically because of this action - @actions/checkout, and the defaults it has.

Lerna (semantic release under the hood) will create an incorrect version bump when used inside github actions.

This happens because by default, the https://github.com/actions/checkout will only fetch the git repository with --depth=1 and will not fetch the tags.

In order to fix this, you need to make the following changes to your workflow:

steps:
  - uses: actions/checkout@v2
+    with:
+      # pulls all commits (needed for lerna / semantic release to correctly version)
+      fetch-depth: "0"

+ # pulls all tags (needed for lerna / semantic release to correctly version)
+ - run: git fetch --depth=1 origin +refs/tags/*:refs/tags/*

Cheers! Hopefully I'll save someone else's time, because for me this has been a real struggle for a while.


I've already created FYIs in lerna/lerna#2542 and semantic-release/semantic-release#1526, since these are the tools I've used when I encountered the issue, but I'd also like to discuss it here.

I've previously used CircleCI and that was never an issue, but ever since I've switched to github actions, this has been secretly hiding & I didn't even notice that there were incorrect version bumps produced - I'm bringing attention to this to hopefully save some time for others, or just get the issue fixed in the first place:D


The fix I found was from here: https://stackoverflow.com/questions/60180630/lerna-always-lists-all-packages-ready-to-publish-when-running-workflow-of-github

and the funny thing is how I found it:

I've noticed that lerna runs & versions incorrectly in my github actions workflow, but once running lerna version locally, everything works fine.

I've checked the outputs of lerna locally and in the CI, and there was a slight difference (red - inside github actions - bad; green - locally - good):

4c4
4c4
< lerna info Assuming all packages changed
---
> lerna info Looking for changed packages since v2.1.5
8,12c8,11
<  - @turbo-schedule/client: 2.1.5 => 2.1.6
<  - @turbo-schedule/common: 2.1.5 => 2.1.6
<  - @turbo-schedule/database: 2.1.5 => 2.1.6
<  - @turbo-schedule/scraper: 2.1.5 => 2.1.6
<  - @turbo-schedule/server: 2.1.5 => 2.1.6
---
>  - @turbo-schedule/common: 2.1.5 => 2.2.0
>  - @turbo-schedule/database: 2.1.5 => 2.2.0
>  - @turbo-schedule/scraper: 2.1.5 => 2.2.0
>  - @turbo-schedule/server: 2.1.5 => 2.2.0

notice the first 3 lines:

< lerna info Assuming all packages changed
---
> lerna info Looking for changed packages since v2.1.5

so I just googled "lerna Assuming all packages changed" and ended up at this stack overflow thread:
https://stackoverflow.com/questions/60180630/lerna-always-lists-all-packages-ready-to-publish-when-running-workflow-of-github

and the author himself found the solution! As explained above, it's because github's @actions/checkout does not fetch all commits & tags!

@ericsciple
Copy link
Contributor

The default is depth: 1, not depth: 0.

checkout@v2 fetches only a single commit by default. It's an intentional perf optimization for the mainline scenario (just need the files). Slow by default has different problems, users would need to opt-in to perf.

The behavior is prominent in the readme. And scenario guidance in the readme, for fetching more history and tags.

I wonder whether there is something more in the log that should be printed.

If downstream tools require more history, would it make sense for the tools to detect whether operating on a shallow repository? That is, whether .git/shallow file exists. It however doesn't solve the problem where tags haven't been fetched. By default the GITHUB_TOKEN is persisted in the git config, so if tags are required, downstream tool could run git fetch.

@kiprasmel kiprasmel changed the title RFC: fetch-depth: 0 and not cloning tags are dangerous defaults RFC: fetch-depth: 1 and not cloning tags are dangerous defaults Apr 15, 2020
@kiprasmel
Copy link
Author

kiprasmel commented Apr 15, 2020

Hey there,

The default is depth: 1, not depth: 0.

Whoops, fixed the title.

checkout@v2 fetches only a single commit by default. It's an intentional perf optimization for the mainline scenario (just need the files). Slow by default has different problems, users would need to opt-in to perf.

Yep, I understand the trade-offs. I guess my problem here was that I wasn't aware of the default.

The behavior is prominent in the readme. And scenario guidance in the readme, for fetching more history and tags.

Yep - the README's great! The problem was that I never went through it in the first place (which is my own fault), not realizing that such issues could come up. This does signal quite a bit about the bad unexpected default on the github actions' side though - this is why I was so frustrated in the first place.

I wonder whether there is something more in the log that should be printed.

Yeah this might be useful, but I'm not sure if this would've helped in this case, because everything seems to work fine and no errors come up. Checking the logs, and specifically the step that github actions are responsible for - pretty unlikely, just like in my case.

Thank you for your responses - let's figure this out!

@ericsciple
Copy link
Contributor

I'm going to add a troubleshooting doc (for other issues). I wonder if it would make sense to add a section related to this issue. Unfortunate since as you mentioned, there is no error message :)

@jasonkuhrt
Copy link

Simpler config might also help, e.g.:

  ...
  with:
    all_commits: true
    all_tags: true

@bastimeyer
Copy link

The behavior is prominent in the readme. And scenario guidance in the readme, for fetching more history and tags.

The "Fetch all tags" command in the readme doesn't always work as intended when it's used for being able to run git describe --tags for example.

The issue is that the initial fetch-depth of this action - if a value greater than 1 is set - doesn't necessarily fetch the correct commits to reach the desired tag(s). Running the custom fetch-all-tags command afterwards with a fetch depth value of just 1 may get all the individual tagged commits, but git won't always be able to trace the commit history back to the "nearest" tag when running git describe --tags, because the initial fetch could have fetched different commits while the custom command didn't fetch any further commits at all.

See my comment with a more detailed description here, where we've just run into this issue:
streamlink/streamlink#2932 (comment)

@ejsmith
Copy link

ejsmith commented Apr 30, 2020

I understand the defaults being optimized for speed and efficiency. But it seems absolutely ridiculous that I need to do a completely separate step / command to get tags for the repository. You allow me to set fetch depth, but then you still add --no-tags so I still gotta do a separate command.

@ejsmith
Copy link

ejsmith commented Apr 30, 2020

Seems pretty logical and reasonable to me that if I set fetch-depth: 0 then I clearly want everything and history matters to me and that you should not add the --no-tags parameter.

@ericsciple
Copy link
Contributor

I like the fetch-depth: 0 suggestion quite a bit. It's simple.

@silasdavis
Copy link

silasdavis commented May 7, 2020

Slow by default has different problems, users would need to opt-in to perf.

But it would not be slow by default. In my experience with large repos it is extremely fast even with a full clone. Apparently Github has a good connection to Github. I suspect this saves about 1-2 seconds on average for checkout time, and for us at least have wasted minutes-to-hours of developer time, multiply that out a bit and it looks like a poor trade-off (ane one none of the other CI providers seem to have made...).

A very large proportion of CI workflows will want to know about tags.

The "Fetch all tags" command in the readme doesn't always work as intended when it's used for being able to run git describe --tags for example.

Indeed git fetch --prune --unshallow is now our boilerplate everywhere because we have VERSION=$(shell git describe --tags) in most of our makefiles.

If downstream tools require more history, would it make sense for the tools to detect whether operating on a shallow repository?

I think it would make more sense for Github Actions to change rather than everything else that already exists.

This seems like a bad default.

@ericsciple
Copy link
Contributor

fyi - i updated v2 so that fetch-depth: 0 now fetches all tags and branches

@ericsciple
Copy link
Contributor

I also updated the README to make the default behavior more prominent

@ericsciple
Copy link
Contributor

Let me know if it's useful to keep this issue open any longer. Otherwise I'm planning to close.

@fregante
Copy link

fregante commented May 31, 2020

@ericsciple Is this still the most efficient way to pull 1 commit and all the tags?

- use: actions/checkout@v2
- run: git fetch --depth=1 origin +refs/tags/*:refs/tags/*

Like OP, I just need the latest tag to verify whether HEAD has been tagged.

@ericsciple
Copy link
Contributor

@fregante yes that looks correct to me

@ericsciple
Copy link
Contributor

Thanks all for the discussion. Sounds like this issue can be closed now. Reopen or open a new issue if any further changes required. Thanks!

simu added a commit to daiboruta/commodore that referenced this issue Jun 15, 2020
Otherwise, actions/checkout@v2 will only fetch the latest commit,
leaving `git describe` unable to produce a useful version, cf.
actions/checkout#100 and
actions/checkout#217
simu added a commit to daiboruta/commodore that referenced this issue Jun 15, 2020
Otherwise, `actions/checkout@v2` will only fetch the latest commit,
leaving `git describe` unable to produce a useful version, cf.
actions/checkout#100 and
actions/checkout#217

Also switch to `actions/checkout@v2` when building the master branch and
release Docker images.
simu added a commit to daiboruta/commodore that referenced this issue Jun 15, 2020
Otherwise, `actions/checkout@v2` will only fetch the latest commit,
leaving `git describe` unable to produce a useful version, cf.
actions/checkout#100 and
actions/checkout#217

Also switch to `actions/checkout@v2` when building the master branch and
release Docker images.
simu added a commit to daiboruta/commodore that referenced this issue Jun 15, 2020
Otherwise, `actions/checkout@v2` will only fetch the latest commit,
leaving `git describe` unable to produce a useful version, cf.
actions/checkout#100 and
actions/checkout#217

Also switch to `actions/checkout@v2` when building the master branch and
release Docker images.
srueg pushed a commit to daiboruta/commodore that referenced this issue Jun 22, 2020
Otherwise, `actions/checkout@v2` will only fetch the latest commit,
leaving `git describe` unable to produce a useful version, cf.
actions/checkout#100 and
actions/checkout#217

Also switch to `actions/checkout@v2` when building the master branch and
release Docker images.
jktjkt added a commit to jktjkt/oopt-gnpy that referenced this issue Jun 8, 2021
We're using PBR, so let's make sure we don't get a package that's marked
as version 0.0.0.

Bug: actions/checkout#217
Change-Id: Icd8264a798f9a1a404e21a9b64317c57662d53fe
jktjkt added a commit to jktjkt/oopt-gnpy that referenced this issue Jun 8, 2021
We're using PBR, so let's make sure we don't get a package that's marked
as version 0.0.0.

Bug: actions/checkout#217
Change-Id: Icd8264a798f9a1a404e21a9b64317c57662d53fe
jktjkt added a commit to Telecominfraproject/oopt-gnpy that referenced this issue Jun 9, 2021
We're using PBR, so let's make sure we don't get a package that's marked
as version 0.0.0.

Bug: actions/checkout#217
Change-Id: Icd8264a798f9a1a404e21a9b64317c57662d53fe
@builtinnya
Copy link

For those who mindlessly pasted the author's fix (like me) and are struggling with why most times git merge fails with fatal: refusing to merge unrelated histories error, delete # pulls all tags part. Just specifying fetch-depth: 0 is enough:

steps:
  - uses: actions/checkout@v2
+    with:
+      # pulls all commits (needed for lerna / semantic release to correctly version)
+      fetch-depth: "0"

- # pulls all tags (needed for lerna / semantic release to correctly version)
- - run: git fetch --depth=1 origin +refs/tags/*:refs/tags/*

git fetch --depth=1 origin +refs/tags/*:refs/tags/* seems to destroy histories of tagged commits.

jayaddison added a commit to jayaddison/pywisetransfer that referenced this issue Jan 2, 2022
…ding to the checked-out ref

By default and for performance reasons, GitHub actions/checkout@v2 intentionally performs a zero-depth clone of the commit ref provided to it.  For our use case, we would also like to have the git tag corresponding to the release available in the local repository clone so that poetry-dynamic-versioning can discover and use this tag name to determine the version label for the package build.

A command to achieve this was discovered at actions/checkout#217 (comment) (and modified slightly to only fetch the relevant tag)
@samuelgfeller
Copy link

Thank you, @builtinnya! I kind of expected this, so I scrolled all the way down; it was worth it.

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

10 participants