Native SSH Key Support by JamesIves · Pull Request #569 · JamesIves/github-pages-deploy-action

JamesIves · 2021-01-13T15:05:48Z

Description

Provide a description of what your changes do.

Implements SSH key support natively to the project.

Testing Instructions

Give us step by step instructions on how to test your changes.

Generate an SSH key: ssh-keygen -t rsa -m pem -b 4096 -C "[email protected]" -N ""
Add the public key as a deploy token in the GitHub repo, give it write access.
Add the private key as a secret using the ssh-key input.
Attempt to deploy, the token should be added and the project will deploy.

Additional Notes

Anything else that will help us test the pull request.

There's a few things that need to be wrapped up, but I wanted to get this to PR sooner as I've been holding up v4 with this.

Fix TODO statements in constants.ts.
Add sshKey to action.yml
Fix README.
Add/fix unit tests for ssh.ts.

Pike

I found that you added the same signature for knownhosts twice, the one I pasted should be OK.

GH only uses the ssh-rsa one in https://github.com/actions/checkout/blob/5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f/src/git-auth-helper.ts#L227, so that should be enough, I guess?

src/ssh.ts

Pike · 2021-01-15T13:02:09Z

src/ssh.ts

+    await mkdirP(sshDirectory)
+
+    appendFileSync(sshKnownHostsDirectory, sshGitHubKnownHostRsa)
+    appendFileSync(sshKnownHostsDirectory, sshGitHubKnownHostDss)


Would this be a problem for folks using the js module api? No idea if that and ssh and messing with the user's knownhosts file would be bad?

It could be yeah. I wonder what a good alternative would be?

Perhaps we always sshToken to be a string or true, and make the boolean skip the token setup and just allow it to context switch to using the ssh endpoints like before? I'd prefer to not have sshKey and ssh to keep things simpler.

Sounds good to me. That would also make the push action re-use ssh auth setups from the checkout action, I guess?

That was another scenario I had in mind.

Co-authored-by: Axel Hecht <[email protected]>

Pike

I found a nit on the readme.

Also, the integration tests need an update?

README.md

JamesIves · 2021-01-18T15:10:15Z

@Pike Issues should be resolved! Although for some reason I'm getting an error trying to resolve a unit test as it is flagging on appendFileSync.

Pike · 2021-01-18T15:27:39Z

@Pike Issues should be resolved! Although for some reason I'm getting an error trying to resolve a unit test as it is flagging on appendFileSync.

I think #421aa39 was on the right track. ssh.ts imports util.ts, which wants existsSync from the mocked fs. So you have to mock both what ssh.ts and util.ts want, unless you mock away util.ts, that is. Which might be just fine?

codecov-io · 2021-01-18T20:36:21Z

Codecov Report

Merging #569 (a41b750) into dev-v4 (0b7a591) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            dev-v4      #569   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         7    +1     
  Lines          175       200   +25     
  Branches        48        50    +2     
=========================================
+ Hits           175       200   +25

Impacted Files	Coverage Δ
src/lib.ts	`100.00% <100.00%> (ø)`
src/ssh.ts	`100.00% <100.00%> (ø)`
src/util.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b7a591...a41b750. Read the comment docs.

JamesIves · 2021-01-20T04:01:10Z

@Pike Issues should be resolved! Although for some reason I'm getting an error trying to resolve a unit test as it is flagging on appendFileSync.

I think #421aa39 was on the right track. ssh.ts imports util.ts, which wants existsSync from the mocked fs. So you have to mock both what ssh.ts and util.ts want, unless you mock away util.ts, that is. Which might be just fine?

Should be good to go now

Pike

lgtm.

Should we have an integration test to reuse the ssh key from, say, a checkout action? Might be also OK as a follow-up, I guess.

JamesIves · 2021-01-20T14:49:39Z

Good call, I'll add those before I merge this. Hoping to do it after work today.

I'll also cut a new beta release with all of the changes so it can be properly tested prior to release.

* Stop checking out workspace (#515) * Stop checking out base branch before deployment, drop option. * Don't check out default branch, as we don't check out base branch, drop option. * Don't stash/unstash as we don't update the workdir, drop preserve option. * Don't init the workspace * Only fetch the remote branch if it exists, only with depth 1. * Rely on previous checkouts to have handled lfs files correctly, drop option. * Update README, action.yml, integration tests * Set up eslint for test files. (#517) * Add DRY_RUN option, passing --dry-run to git push. (#526) See #499 for the proposal. * Simplifies Token Setup (#530) * Token simplification * Access Token / Github Token -> Token * Oops * Typos * Update README.md * Update README.md * Update action.yml Co-authored-by: Axel Hecht <[email protected]> * Update README.md Co-authored-by: Axel Hecht <[email protected]> * Update README.md Co-authored-by: Axel Hecht <[email protected]> * Adjust codeql action to latest recommendations (#540) Also, add the dev and release branches, and drop master. * Add workflow to update build and node_modules on release branches (#541) * Stores username/email in secrets * Removing stale bot integration * Test current code base as an integration test for PRs and pushes (#505) * Add a build step to create lib and node_modules artifact * Run integration test with built dist and current SHA as base For pull requests, the github.sha is the sha of the merge to the target branch, not the head of the PR. Special case that. * Use v2 checkout, and DRY_RUN for the integration test. I also made the branches more generic, as there are now more of them. * Fix #536, don't push at all on dryRun Also add tests for dryRun and singleCommit and generateBranch code flows. * Try to fix dryRun on new remote branches, refactor fetch * Try to fix dryRun, only fetch if origin branch exists * Refactor worktree setup to include branch generation and setup for singleCommit This is a continuation of the no-checkout work, and sadly suggested pretty intensive changes. * Set up git config to fix tests, also make debugging easier * Add matrix for existing and non-existing branch * Add matrix for singleCommit and not * Drop GITHUB_TOKEN, add DRY_RUN to action.yml * When deploying existing branch, add a modifcation and deploy again * Force branch checkout to work in redeployment scenarios * Make singleCommit easier to see in job descriptions * Review comments * Add a test-only property to action to test code paths with remote branch. * Introduce TestFlag enum to signal different test scenarios to unit tests * Fix util.test.ts * Update worktree.ts * Fix a few nits in tests and automation. Don't try to wordcount ls-rem… (#546) * Fix a few nits in tests and automation. Don't try to wordcount ls-remote. Nits in tests are around undoing changes made to the environment, and to not modify the checkout. * Describe suite with empty SHA * Lowercase Inputs (#547) * Lowercases inputs * Adjusts workflow tests and deployment_status * Use multi-line string for clean-exclude patterns. (#553) As this change is subtle, I'm taking the opportunity to change the underscore for the hyphen, which makes it less likely that users of this action will just pass in an old json array. * Hyphenate inputs and outputs, add step output, fix #558 (#559) * Hyphenate inputs and outputs, add step output, fix #558 I've also tried to make the clean docs a bit clearer, and consistent about clean being on my default. Still not totally happy with the intro of the docs there, though. * Add testing of step outputs to build integration tests * Security Docs * Integration tests * Revert "Integration tests" This reverts commit 639ff53. * Native SSH Key Support (#569) * SSH Key Support 🔑 * Update ssh.ts * Update src/ssh.ts Co-authored-by: Axel Hecht <[email protected]> * README fixes/etc * Unit Tests & README * ssh key * Update README.md * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update integration.yml Co-authored-by: Axel Hecht <[email protected]> * Deployment Issues (#583) * Update git.ts * Tests * Update git.ts * Formatting * Update src/git.ts Co-authored-by: Axel Hecht <[email protected]> * TestFlag * Logging * Update git.ts Co-authored-by: Axel Hecht <[email protected]> * Codespace Support (#584) * Add files via upload * Update README.md * Add files via upload * Update README.md * Update README.md * Update README.md * Update README.md * Update README.md * SSH Issues (#588) * Unsets Persisted Credentials (#587) * Persist * Config Setup/Tests * Assets * Update git.ts * Spacing * Update integration.yml * Update README.md Co-authored-by: Axel Hecht <[email protected]>

SSH Key Support 🔑

72b980b

JamesIves changed the title ~~SSH Key Support 🔑~~ Native SSH Key Support Jan 13, 2021

Update ssh.ts

b1b3962

JamesIves added the version 4 Issues related to version 4 of this action. label Jan 15, 2021

Pike reviewed Jan 15, 2021

View reviewed changes

JamesIves and others added 2 commits January 16, 2021 08:59

Update src/ssh.ts

b6fcc5b

Co-authored-by: Axel Hecht <[email protected]>

README fixes/etc

d33c95b

Pike reviewed Jan 18, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

JamesIves added 7 commits January 18, 2021 09:52

Unit Tests & README

c02aaad

ssh key

bb672ee

Update README.md

bda256a

Update ssh.test.ts

510f083

Update ssh.test.ts

db4f39f

Update ssh.test.ts

421aa39

Update ssh.test.ts

0ed249c

JamesIves added 2 commits January 18, 2021 15:33

Update ssh.test.ts

d1e4805

Update ssh.test.ts

a41b750

Pike approved these changes Jan 20, 2021

View reviewed changes

Update integration.yml

31957e7

JamesIves merged commit 64eb711 into dev-v4 Jan 21, 2021

JamesIves deleted the ssh-deploy-key-token branch January 21, 2021 14:08

Uh oh!

Comments

Conversation

JamesIves commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pike left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pike Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

JamesIves Jan 16, 2021

Choose a reason for hiding this comment

Uh oh!

Pike Jan 17, 2021

Choose a reason for hiding this comment

Uh oh!

Pike left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JamesIves commented Jan 18, 2021

Uh oh!

Pike commented Jan 18, 2021

Uh oh!

codecov-io commented Jan 18, 2021

Codecov Report

Uh oh!

JamesIves commented Jan 20, 2021

Uh oh!

Pike left a comment

Choose a reason for hiding this comment

Uh oh!

JamesIves commented Jan 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JamesIves commented Jan 13, 2021 •

edited

Loading