- 
                Notifications
    You must be signed in to change notification settings 
- Fork 161
Add check to see if key already exists locally before querying keyservers #209
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
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
          
        
      | 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).