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

Prioritize git environment variables from parent process #3885

Merged
merged 1 commit into from
Jul 12, 2017

Conversation

monder
Copy link
Contributor

@monder monder commented Jul 10, 2017

Summary

If there are any ENV variables specified for the parent process - use them.
The main reason for this is that our CI uses GIT_SSH_COMMAND env to specify ssh keys to use for cloning and not via ssh-agent.
For example for github the user is always git@ and it is obviously possible to make fake users in ssh config, etc...
But there should be an option to customize GIT_SSH_COMMAND directly.

Test plan

Before:

$ GIT_SSH_COMMAND="ssh -i ~/git.pem -F /dev/null" yarn install
yarn install v0.27.5
[1/4] Resolving packages...
[2/4] Fetching packages...
error Command failed.
Exit code: 128
Command: git
Arguments: clone git@github.com:monder/test.git /home/monder/.cache/yarn/v1/.tmp/f13ca8a124faac37abd837f21d8375bf
Directory: /tmp/test
Output:
Cloning into '/home/monder/.cache/yarn/v1/.tmp/f13ca8a124faac37abd837f21d8375bf'...
Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

After:

$ GIT_SSH_COMMAND="ssh -i ~/git.pem -F /dev/null" yarn install
yarn install v0.27.5
[1/4] Resolving packages...
[2/4] Fetching packages...
warning fsevents@1.1.2: The platform "freebsd" is incompatible with this module.
info "fsevents@1.1.2" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 108.73s.

@monder monder force-pushed the env branch 2 times, most recently from a18f476 to 48edbd8 Compare July 10, 2017 17:51
@kaylie-alexa
Copy link
Member

kaylie-alexa commented Jul 11, 2017

Looking at this previous PR (#3633), it looks like your changes may reintroduce bugs. Can you try appending the args to GIT_SSH_COMMAND instead of overriding them?

@monder
Copy link
Contributor Author

monder commented Jul 11, 2017

The variables will be overridden only if they are defined in shell, which is not a default. By default all three variables will be appended to the env as previously. I've tested it with the repo specified in #3633 and it works.
Also I cannot make any assumptions to merge GIT_SSH_COMMAND - it may be pointing to a wrapper script that is not even ssh.

@kaylie-alexa
Copy link
Member

gotcha, thanks for explaining and testing the previous issue :) Yeah I guess at this execution level, the user's intent is well defined. If ci passes, looks good to me!

@BYK
Copy link
Member

BYK commented Jul 12, 2017

I'm still a bit concerned about this reverting the freeze fixes but let's wait and see if we get any complaints.

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.

3 participants