tools: add custom private key option for release.sh by krydos · Pull Request #14401 · nodejs/node · GitHub

Hi,
I just noticed issue here - #14378 and tried to solve it.
So I've added -i option for release.sh that allows user to specify non-default private key for ssh and scp commands.

I've tested script manually with some debug echo statements since I'm not sure we have any tests for our tools (or idk about them).

One more thing is shift $((OPTIND-1)) right after while loop. It's important to save variable such as $1, $2 etc, that are used in the script. So these variables should not be broken.

If anyone could help me to test this script it would be great :)

Checklist

commit message follows commit guidelines

Affected core subsystem(s)

tools

Oh nice! Thanks!

As I said I'm not this familiar with bash but I can certainly try using this for 8.2.1 (#14399).

I think the main thing would be to ensure this works with -s, which uses a much more rudimentary flag detection mechanism.

ah, didn't notice -s there... I hope it works because of shift $((OPTIND-1)) but I'm not sure.
If no one will against getopts I guess it would be nice to rewrite -s flag to the way how -i works

So, I used this for 8.2.1 and it worked just fine! 😄

(I did not need to use -s though.)

Nice work @krydos! It'd be nice to do -s in the same way but that's not a big deal. I'll review and test shortly and be back with feedback.

-s doesn't work anymore with this, it gets chopped off completely. I guess because it's technically an option and getopts includes it in OPTIND even though it's not handled by the case

so could you try pulling the -s up into the getopts handling? it always needs a version number argument.

Sure, I'll try to convert -s option to getopts as soon as possible

I've changed how -s is parsed. It took too much time to find a way how to show custom message when option is passed but argument for this option is not... And didn't find solution yet :(

Previously in case nothing is passed to -s option you could see message Please supply a version string to sign. Now you will see this one ./release.sh: option requires an argument -- s (default one produced by getopts). If it's ok then great, if not I will try to come up with something else.

And again, I tried to debug it with multiple echo statements but I was unable to debug if script in a whole works correctly or not.
If someone could test it, it would sooo great.
I hope I didn't break anything 😟

Hmm, the script doesn't actually seem to run now? All variants with valid options just seem to return from the script without doing anything. :/

ah, excuse me, I didn't really get the purpose of $OPTERR and used it wrongly.
Should be working from now.

Will re-review

I promoted the armv6 binaries for 8.2.1 with the latest version of this.

Both -i and -s worked just fine. I wasn't able to test without -i though.

@rvagg @jasnell could ya'll re-review?

@nodejs/build & @jasnell can someone with bash knowledge please review / sign off on this?

If you want a nicer error message you can do this:

diff --git a/tools/release.sh b/tools/release.sh
index 4225be4cc7..fcb5ef1242 100755
--- a/tools/release.sh
+++ b/tools/release.sh
@@ -16,7 +16,7 @@ signcmd=dist-sign
 customsshkey="" # let ssh and scp use default key
 signversion=""
 
-while getopts "i:s:" option; do
+while getopts ":i:s:" option; do
     case "${option}" in
         i)
             customsshkey="-i ${OPTARG}"
@@ -25,6 +25,11 @@ while getopts "i:s:" option; do
             signversion="${OPTARG}"
             ;;
         \?)
+            echo "Invalid option -$OPTARG."
+            exit 1
+            ;;
+        :)
+            echo "Option -$OPTARG takes a parameter."
             exit 1
             ;;
     esac

up to you whether it's worth it though.

Both -i and -s worked just fine. I wasn't able to test without -i though.

@Fishrock123 FWIW you can add the IdentityFile option to the ~/.ssh/config entry for direct.nodejs.org, something like this should work (then you could test without the -i):

Host direct.nodejs.org
     User dist
     IdentityFile ~/.ssh/custom_id_rsa

Looks fine with or without the proposed fix.

@gibfahn thank you for the help. I didn't know that colon at the beginning of the arguments definition switch getopts to silent error reporting mode. Very cool!

Add -i option for release.sh that allows user to specify non-default private key for ssh and scp commands. Change argument parsing to getopts.

Landed in 237a42d

Add -i option for release.sh that allows users to specify non-default private key for ssh and scp commands. Change argument parsing to getopts. PR-URL: #14401 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Add -i option for release.sh that allows users to specify non-default private key for ssh and scp commands. Change argument parsing to getopts. PR-URL: nodejs/node#14401 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Add -i option for release.sh that allows users to specify non-default private key for ssh and scp commands. Change argument parsing to getopts. PR-URL: #14401 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jul 20, 2017

krydos force-pushed the custom-ssh-key-for-release-sh branch from c0b8c45 to 3648684 Compare July 21, 2017 11:08

jasnell previously approved these changes Jul 21, 2017

View reviewed changes

krydos force-pushed the custom-ssh-key-for-release-sh branch from 3648684 to 8f6897f Compare July 24, 2017 19:39

Fishrock123 approved these changes Jul 28, 2017

View reviewed changes

gibfahn reviewed Aug 14, 2017

View reviewed changes

gibfahn approved these changes Aug 14, 2017

View reviewed changes

krydos force-pushed the custom-ssh-key-for-release-sh branch from 8f6897f to f24235b Compare August 18, 2017 06:56

tools: add custom private key option

688d22e

Add -i option for release.sh that allows user to specify non-default private key for ssh and scp commands. Change argument parsing to getopts.

krydos force-pushed the custom-ssh-key-for-release-sh branch from f24235b to 688d22e Compare August 18, 2017 07:08

gibfahn approved these changes Aug 19, 2017

View reviewed changes

BridgeAR closed this Aug 27, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the land-on-v6.x label Sep 20, 2017

MylesBorins mentioned this pull request Sep 20, 2017

v6.11.4 proposal #15506

Merged

tools: add custom private key option for release.sh by krydos · Pull Request #14401 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: add custom private key option for release.sh - #14401

tools: add custom private key option for release.sh#14401krydos wants to merge 1 commit intonodejs:masternodejs/node:masterfrom krydos:custom-ssh-key-for-release-shkrydos/node:custom-ssh-key-for-release-shCopy head branch name to clipboard

Conversation

krydos commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Fishrock123 commented Jul 20, 2017

Uh oh!

Fishrock123 commented Jul 20, 2017

Uh oh!

krydos commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Jul 20, 2017

Uh oh!

rvagg commented Jul 20, 2017

Uh oh!

rvagg commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krydos commented Jul 21, 2017

Uh oh!

krydos commented Jul 21, 2017

Uh oh!

Fishrock123 commented Jul 24, 2017

Uh oh!

krydos commented Jul 24, 2017

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jul 31, 2017

Uh oh!

Fishrock123 commented Aug 14, 2017

Uh oh!

gibfahn Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

krydos commented Aug 18, 2017

Uh oh!

BridgeAR commented Aug 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tools: add custom private key option for release.sh#14401
krydos wants to merge 1 commit into
nodejs:masterfrom
krydos:custom-ssh-key-for-release-sh

krydos commented Jul 20, 2017 •
edited
Loading

krydos commented Jul 20, 2017 •
edited
Loading

rvagg commented Jul 21, 2017 •
edited
Loading

gibfahn commented Aug 14, 2017 •
edited
Loading