Skip to content
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.

Sign up for GitHub

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

installFromYum: give more detailed error messages on gpg errors #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

installFromYum: give more detailed error messages on gpg errors #71

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,12 +821,59 @@ def installFromYum(targets, mounts, progress_callback, cachedir):
rv = p.wait()
stderr.seek(0)
stderr = stderr.read()
gpg_uncaught_error = 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of defining these variables and then raising the ErrorInstallingPackage exception later - could we not simply raise the exception directly at the point we find something in the log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot recall precisely, but:

  • (likely original reason) to keep the structure of the existing code; notably, there was already an exception raised with a generic message, and all that needed to be changed was the message
  • in line with that, it allows to keep the "yum exited" log, which would have to be duplicated, or wrapped with the exception in a helper (and delegating the raise to a helper function causes its own problems)
  • additionally it allows to make sure to use the most pertinent message, regardless of the matching order, which in turn is constrained by the order in which strings appear. Maybe not really necessary for this set of messages, but considering this is ad-hoc parsing of a program's output, including parsing of its crashes, it may be useful to retain this flexibility

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why instead of using a lot of flags variable you don't just set a string variable errmsg to save the specific error and use it? You could initialize it at default generic message and change accordingly based on the specific error.

gpg_error_pubring_import = 0
gpg_error_not_signed = 0
gpg_error_bad_repo_sig = 0
gpg_error_rpm_missing_key = None
gpg_error_rpm_not_signed = None
gpg_error_rpm_not_found = None
if stderr:
logger.log("YUM stderr: %s" % stderr.strip())

if stderr.find(' in import_key_to_pubring') >= 0:
gpg_error_pubring_import = 1
# add any other instance of uncaught GpgmeError before this like
elif stderr.find('gpgme.GpgmeError: ') >= 0:
gpg_uncaught_error = 1

elif re.search("Couldn't open file [^ ]*/repodata/repomd.xml.asc", stderr):
# would otherwise be mistaken for "pubring import" !?
gpg_error_not_signed = 1
elif stderr.find('repomd.xml signature could not be verified') >= 0:
gpg_error_bad_repo_sig = 1

else:
match = re.search("Public key for ([^ ]*.rpm) is not installed", stderr)
if match:
gpg_error_rpm_missing_key = match.group(1)
match = re.search("Package ([^ ]*.rpm) is not signed", stderr)
if match:
gpg_error_rpm_not_signed = match.group(1)
match = re.search(r" ([^ ]*): \[Errno [0-9]*\] No more mirrors to try", stderr)
if match:
gpg_error_rpm_not_found = match.group(1)

if rv:
logger.log("Yum exited with %d" % rv)
raise ErrorInstallingPackage("Error installing packages")
if gpg_error_pubring_import:
errmsg = "Signature key import failed"
elif gpg_uncaught_error:
errmsg = "Cryptography-related yum crash"
elif gpg_error_not_signed:
errmsg = "No signature on repository metadata"
elif gpg_error_bad_repo_sig:
errmsg = "Repository signature verification failure"
elif gpg_error_rpm_missing_key:
errmsg = "Missing key for %s" % (gpg_error_rpm_missing_key,)
elif gpg_error_rpm_not_signed:
errmsg = "Package not signed: %s" % (gpg_error_rpm_not_signed,)
elif gpg_error_rpm_not_found:
# rpm not found or corrupted/re-signed/etc
errmsg = "Cannot find valid rpm for %s" % (gpg_error_rpm_not_found,)
else:
errmsg = "Error installing packages"
raise ErrorInstallingPackage(errmsg)

shutil.rmtree(os.path.join(mounts['root'], cachedir))

Expand Down