-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Import the zypper GPG key before templating the repo #6410
Conversation
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 don't love using a before notification here, we should make it actually idempotent like apt_repository. Otherwise you could get things out of sync if you monkey with the files manually.
@coderanger I'm trying to find a better way to handle that import, but getting a reliable list is hard. |
d181d6e
to
a717477
Compare
b6415d7
to
8bb1b8a
Compare
|
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.
Looks good overall, just some cleanup-y things.
One structural thing is this doesn't allow importing a key by fingerprint. Is that just not a thing the SuSE community does?
end | ||
|
||
declare_resource(:execute, "import gpg key from #{new_resource.gpgkey}") do | ||
command "/bin/rpm --import #{new_resource.gpgkey}" |
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 can use the cache file to avoid downloading it twice.
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.
Oh nice catch
# | ||
# @return [boolean] is the key already known by rpm | ||
def key_installed?(key_path) | ||
so = shell_out("rpm -qa gpg-pubkey*").run_command |
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.
the run_command
isn't needed, that's handed by our mixin.
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.
That was cargo culted from apt. I'll get that cleaned up there as well later
# | ||
# @return [String] the fingerprint of the key | ||
def key_fingerprint(key_path) | ||
so = shell_out("gpg --with-fingerprint #{key_path}").run_command |
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 should be shell_out!
since a failure is not expected, also no run_command
.
8bb1b8a
to
9df8f9e
Compare
I'm not sure about the key importing via fingerprint to be honest. The goal here was to mostly just maintain compat with the behavior of the existing zypper cookbook. We have that and now we can take local keys as well, which is a nice addition thanks to the apt cargo culting. |
Next step here is to make sure it works on various versions of SLES and not just opensuse leap, which is where I did my testing. Once that's done I'll get testing on the various methods here. |
Works for me re: importing from fingerprint, happy to YAGNI it until someone complains :) |
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.
Looks good to me assuming it passes whatever functional tests you can throw at it.
Converges were successful for the nginx repo on: openSUSE Leap 42.2 |
This prevents failures when the key is unknown. This way is *simple*, but I'm not a fan of the way the :before notification results in double template logging. I'm open to a better way that provides some idempotency to the key import. Signed-off-by: Tim Smith <[email protected]>
Signed-off-by: Tim Smith <[email protected]>
GPG import doesn't actually work here. Signed-off-by: Tim Smith <[email protected]>
Signed-off-by: Tim Smith <[email protected]>
Download the key to the cache Grab the key's fingerprint Check to see if that fingerprint is in the RPM database Add it if it's not Signed-off-by: Tim Smith <[email protected]>
Signed-off-by: Tim Smith <[email protected]>
Avoid double downloading the key Signed-off-by: Tim Smith <[email protected]>
Signed-off-by: Tim Smith <[email protected]>
Also fix the cookbook_file lookup logic missing a method it needed for file based keys Signed-off-by: Tim Smith <[email protected]>
1e643c3
to
0713896
Compare
Signed-off-by: Tim Smith <[email protected]>
0713896
to
7ec3601
Compare
This prevents failures when the key is unknown.
This way is simple, but I'm not a fan of the way the :before notification results in double template logging. I'm open to a better way that provides some idempotency to the key import.
Signed-off-by: Tim Smith [email protected]