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

Fetch tags for cached git repo #6393

Merged
merged 1 commit into from
Sep 24, 2018
Merged

Fetch tags for cached git repo #6393

merged 1 commit into from
Sep 24, 2018

Conversation

luwes
Copy link
Contributor

@luwes luwes commented Sep 15, 2018

Summary

Fixes #6256

This change fixes an issue where the cached git repo was not updated with the latest tags.
As a result yarn would exit with the git archive command -> fatal: not a tree object.

@ramasilveyra
Copy link
Contributor

ramasilveyra commented Sep 17, 2018

This is great, thanks! This error is annoying. I think that this issue was fixed in an old version of yarn and then it got broken again.

Thinking on how to add an e2e test for this. Maybe we should:

  1. create a private GH repo on the yarn org with a dumb package as git tag.
  2. create a fake GH user and give access to this private repo.
  3. add the ssh key of this user to the CI.
  4. and run this new test suite only with a env flag (ENABLE_GIT_PRIVATE_TEST=true yarn test or GIT_PRIVATE_REPO=yarn/private-pkg#1.0.0 yarn test).

The new GH user is for security concerns since the yarn CI is public, the idea is to expose a SSH key with a granular access and from a non personal GitHub account.

Obviously we will need help from the yarn maintainers for this.

Or maybe there is better way? maybe just test Git.fetch() more?

@luwes
Copy link
Contributor Author

luwes commented Sep 20, 2018

@ramasilveyra thanks for providing a potential test plan for this!

I think this could work. The only thing I am concerned about is that this issue only occurs on subsequent installs (like when upgrading the version of the git pkg). Yarn will initially do a git clone which will include all tags from the remote. Do you think it would be possible to publish a new version of the dummy pkg in the test? Another option would be to go in the cached repo and delete the dummy pkg and then run the test.

On a side note, it's possible to get around this issue by adding this global git config.

git config --global --add remote.origin.fetch "+refs/tags/*:refs/tags/*"

This will automatically fetch ALL tags from the remote when Yarn pulls in the latest changes in the cached repo. But at the same every git fetch or git pull on your machine will pull in ALL git tags from the remotes.

@luwes
Copy link
Contributor Author

luwes commented Sep 22, 2018

Does this fix make sense @arcanis ?

That git pull on the cached repo should result in the same repo if a git clone was done.
Currently this isn't the case, that added git fetch --tags will include all tags...

@bestander
Copy link
Member

I think it makes sense, thanks @luwes for finding the root cause.
To keep things going let's merge it while PR is fresh.
e2e test could be hard to do but if you can come up with something we can run on CI it would be great

@bestander bestander merged commit 5682d55 into yarnpkg:master Sep 24, 2018
ramasilveyra added a commit to ramasilveyra/yarn that referenced this pull request Sep 24, 2018
ramasilveyra added a commit to ramasilveyra/yarn that referenced this pull request Sep 24, 2018
ramasilveyra added a commit to ramasilveyra/yarn that referenced this pull request Sep 24, 2018
@ramasilveyra
Copy link
Contributor

Woohoo, this landed 🎉!

Yes is true, e2e test will be difficult.

Thanks for the clarification @luwes!

FYI: last night I was trying to create an integration test for this by running a local git server in a temp dir: master...ramasilveyra:test/fetch-tags-cached-git
but i'm failing at creating the environment or git settings that makes git pull not to fetch the new git tags, in my machine git pull is fetching the git tags. Probably during the week I will give another try, maybe is some git config setting or my git version.

ramasilveyra added a commit to ramasilveyra/yarn that referenced this pull request Sep 24, 2018
ramasilveyra added a commit to ramasilveyra/yarn that referenced this pull request Sep 24, 2018
ramasilveyra added a commit to ramasilveyra/yarn that referenced this pull request Sep 24, 2018
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

Successfully merging this pull request may close these issues.

Yarn cache bug w/ Github URL packages
3 participants