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

fix 2083 supportsArchive can be changed with nothing beeing cloned #2431

Merged
merged 3 commits into from
Jan 20, 2017

Conversation

Congelli501
Copy link
Contributor

@Congelli501 Congelli501 commented Jan 11, 2017

Fix bug #2083
The git cloner could start using archive mode and switch back to local clone mode without cloning anything.
As a result, comands are spawned with a bad (non existent) cwd, leading to a ENOENT error.
This error is ms-interpreted a the lack of the git binary (https://github.com/yarnpkg/yarn/blob/master/src/util/child.js#L41)

Test plan

To reproduce the fixed bug, you need to add a dependency based on:

  • git+ssh (I don't know if git+https will trigger the bug)
  • locked on a commit, not a tag
  • targeting a git server that supports the "archive" command (ie: not github). With its default configuration, gitlab has the necessary support.

This package.json:

{
  "dependencies": {
    "test-project": "git+ssh://git@<gitlab server url>:22/<project path>.git"
  }
}

I can't provide a git server with a node project and a "public" private key to clone the project from CI tests.
Github won't work as it doesn't support the git archive command.

This pull request is the rebased version of #2320

@Congelli501
Copy link
Contributor Author

The MacOS test fails, but it seems unrelated to that commit.

To reproduce the fixed bug, you need to add a dependency based on:

  • git+ssh (I don't know if git+https will trigger the bug)
  • locked on a commit, not a tag
  • targeting a git server that supports the "archive" command (ie: not github). With its default configuration, gitlab has the necessary support.

This package.json:

{
  "dependencies": {
    "test-project": "git+ssh://git@<gitlab server url>:22/<project path>.git"
  }
}

Can reproduce the bug if used with the right server.
Unfortunately, I don't know any public nodejs project hosted on a gitlab server.

@bestander
Copy link
Member

Don't mind Travis/MacOS build, it is a bit flaky.
Could you add the test you've described to https://github.com/yarnpkg/yarn/blob/master/__tests__/package-resolver.js or https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/install/integration.js?

We need to make sure it does not get broken later

@Congelli501
Copy link
Contributor Author

I can't provide a git server with a node project and a "public" private key to clone the project from CI tests.
Github won't work as it doesn't support the git archive command.

This is necessary to trigger the bug, and create a test.

@bestander
Copy link
Member

ok then, lets not waste time on the complex test and merge it then.

Could you instead add a comment to the code explaining why the fetch is there.

As for the PR, considering that no test is added can you add "Test plan" section to the PR description?
With a test scenario that verifies what the PR fixes.
This is very helpful for people to trace down why the changes are added and how to test them in the future.

@Congelli501
Copy link
Contributor Author

I don't know if you got notified, so just in case, I edited my commit & the PR description.

@bestander bestander merged commit 8482400 into yarnpkg:master Jan 20, 2017
@bestander
Copy link
Member

Awesome, thanks for the work!

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.

2 participants