-
Notifications
You must be signed in to change notification settings - Fork 2.4k
add ssh support #163
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
add ssh support #163
Conversation
| export const RepositoryPath = | ||
| (process.env['STATE_repositoryPath'] as string) || '' | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used during POST to cleanup the files on disk
| sshCommand += ` -o "UserKnownHostsFile=$RUNNER_TEMP/${path.basename( | ||
| this.sshKnownHostsPath | ||
| )}"` | ||
| this.git.setEnvironmentVariable('GIT_SSH_COMMAND', sshCommand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GIT_SSH_COMMAND takes precedence over git config core.sshCommand
|
|
||
| // Configure core.sshCommand | ||
| if (this.settings.persistCredentials) { | ||
| await this.git.config(SSH_COMMAND_KEY, sshCommand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this runs git config --local core.sshCommand "ssh -i path-to-key -o etc"
thboop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
7ccce3d to
90142f5
Compare
90142f5 to
ba318a1
Compare
db05968 to
8020685
Compare
| # with the local git config, which enables your scripts to run authenticated git | ||
| # commands. The post-job step removes the PAT. | ||
| # | ||
| # We recommend creating a service account with the least permissions necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: We recommend generating the PAT from a service account with only the necessary permissions.
may read a little better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will follow up on next pr
| token: '' | ||
|
|
||
| # Whether to persist the token in the git config | ||
| # SSH key used to fetch the repository. SSH key is configured with the local git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The SSH Key is configured...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix in next pr
| # config, which enables your scripts to run authenticated git commands. The | ||
| # post-job step removes the SSH key. | ||
| # | ||
| # We recommend creating a service account with the least permissions necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will follow up on next pr
| # recursively checkout submodules. | ||
| # | ||
| # When the `ssh-key` input is not provided, SSH URLs beginning with | ||
| # `git@github.com:` are converted to HTTPS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏅
7b6365b to
d5e7ec5
Compare
| const configureAuth_copiesUserKnownHosts = | ||
| 'configureAuth copies user known hosts' | ||
| it(configureAuth_copiesUserKnownHosts, async () => { | ||
| if (!sshPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: consider a helper function for this
| ).toString() | ||
| expect(actualSshKeyContent).toBe(settings.sshKey + '\n') | ||
| if (!isWindows) { | ||
| expect((await fs.promises.stat(actualSshKeyPath)).mode & 0o777).toBe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Comment what this is or create a constant with a clear name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will follow up on next pr
| } | ||
| ) | ||
|
|
||
| const configureSubmoduleAuth_doesNotConfigureUrlInsteadOfWhenPersistCredentialsTrueAndSshKeySet = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great coverage across all of these different scenarios!
thboop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
8e1d16e to
f393882
Compare
related to ADR #156