-
Notifications
You must be signed in to change notification settings - Fork 146
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 check to see if key already exists locally before querying keyservers #209
Conversation
bin/import-release-team-keyring
Outdated
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} --no-tty --keyserver "hkp://$server" $OPTIONS --display-charset utf-8 --recv-keys "$key" && break | ||
done | ||
for server in $SERVERS; do | ||
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key -with-colons > /dev/null 2>&1 || \ |
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.
I think there's a missing -
in -with-colons
as documented in gpg manpage
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key -with-colons > /dev/null 2>&1 || \ | |
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key --with-colons > /dev/null 2>&1 || \ |
As of the check by itself, I ran it in my machine and it worked perfectly |
Added the automatic import to the |
bin/install
Outdated
@@ -241,6 +241,9 @@ download_and_verify_checksums() { | |||
export GNUPGHOME="${ASDF_DIR:-$HOME/.asdf}/keyrings/nodejs" | |||
fi | |||
|
|||
# Automatically add needed PGP keys | |||
source "$(dirname "$0")/import-release-team-keyring" |
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.
nitpick: no need for source
here. "$(dirname "$0")/import-release-team-keyring"
is sufficient on its own.
bin/import-release-team-keyring
Outdated
$gnugp_verify_command_name --with-colons --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key > /dev/null 2>&1 || \ | ||
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} --no-tty --keyserver "hkp://$server" $OPTIONS --display-charset utf-8 --recv-keys "$key" && break | ||
done | ||
# fi |
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.
commented out code. Please remove.
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} --no-tty --keyserver "hkp://$server" $OPTIONS --display-charset utf-8 --recv-keys "$key" && break | ||
done | ||
for server in $SERVERS; do | ||
$gnugp_verify_command_name --with-colons --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key > /dev/null 2>&1 || \ |
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.
Couldn't this line be moved above line 42?
Something like this:
for key in $KEYS; do
# Halt fetching this key if we already have it
$gnugp_verify_command_name --with-colons --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} -k $key > /dev/null 2>&1 && break
# Otherwise loop through servers until we find it
for server in $SERVERS; do
$gnugp_verify_command_name --no-default-keyring --keyring ${ASDF_NODEJS_KEYRING} --no-tty --keyserver "hkp://$server" $OPTIONS --display-charset utf-8 --recv-keys "$key" && break
done
done
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.
I'd originally tried that but got a weird gnupg_verify_command_name: unbound variable
error that I didn't get when I had it in the inner loop. I futzed with it a bit but ultimately opted to go with working code vice spending more time down the rathole troubleshooting further.
Thanks for the PR @vadave ! I think this is a nice improvement. I left a few comments, let me know if you have any questions. |
Tested this locally and it worked perfectly. Merging. We may want to add some tests around the logic in the |
Thanks for the PR @vadave ! |
Addresses issue #208. Checks to see if the key exists in the keyring. If the check is successful, it bypasses the remote call. This vastly reduces the time needed to execute this script on subsequent runs (which is helpful when you're using something like ansible to manage asdf/plugin configs).