Skip to content

Commit

Permalink
Format messages for consistency
Browse files Browse the repository at this point in the history
- no full stop at the end of a report message title
- no word please (technical writers' rule)
- boot loader => bootloader
- unique report message ids
- fix typos
- add missing context where messages are too short and vague
- make TASK tense consistent
- ensure each Action has a TASK level log message
- ensure every Action prints what it's done
  • Loading branch information
bocekm committed Dec 3, 2024
1 parent dbcd745 commit 6bd4220
Show file tree
Hide file tree
Showing 73 changed files with 370 additions and 344 deletions.
6 changes: 3 additions & 3 deletions convert2rhel/actions/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ def run(self, successes=None, failures=None, skips=None):
title="Skipped action",
description="This action was skipped due to another action failing.",
diagnosis=diagnosis,
remediations="Please ensure that the {} check passes so that this Action can evaluate your system".format(
remediations="Ensure that the {} check passes so that this Action can evaluate your system".format(
utils.format_sequence_as_message(failed_deps)
),
)
Expand All @@ -586,7 +586,7 @@ def run(self, successes=None, failures=None, skips=None):

# Categorize the results
if action.result.level <= STATUS_CODE["WARNING"]:
logger.info("{} has succeeded".format(action.id))
logger.info("The {} action has succeeded.".format(action.id))
successes.append(action)

if action.result.level > STATUS_CODE["WARNING"]:
Expand Down Expand Up @@ -742,7 +742,7 @@ def run_pre_actions():
# When we call check_dependencies() or run() on the first Stage
# (system_checks), it will operate on the first Stage and then recursively
# call check_dependencies() or run() on the next_stage.
pre_ponr_changes = Stage("pre_ponr_changes", "Making recoverable changes")
pre_ponr_changes = Stage("pre_ponr_changes", "Make recoverable changes")
system_checks = Stage("system_checks", "Check whether system is ready for conversion", next_stage=pre_ponr_changes)

try:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def run(self):
super(ListNonRedHatPkgsLeft, self).run()
loggerinst.task("List remaining non-Red Hat packages")

loggerinst.info("Listing packages not signed by Red Hat")
loggerinst.info("Listing packages not signed by Red Hat.")
non_red_hat_pkgs = get_installed_pkgs_w_different_key_id(system_info.key_ids_rhel)
if not non_red_hat_pkgs:
loggerinst.info("All packages are now signed by Red Hat.")
Expand Down
62 changes: 36 additions & 26 deletions convert2rhel/actions/conversion/preserve_only_rhel_kernel.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def run(self):
non-RHEL kernel(s) conflicted with the available RHEL kernels.
"""
super(InstallRhelKernel, self).run()
loggerinst.info("Verifying that RHEL kernel has been installed")
loggerinst.task("Verify RHEL kernel installation")

rhel_kernels = pkghandler.get_installed_pkgs_by_key_id(system_info.key_ids_rhel, name="kernel")

Expand Down Expand Up @@ -68,8 +68,8 @@ class FixInvalidGrub2Entries(actions.Action):

def run(self):
"""
On systems derived from RHEL 8 and later, /etc/machine-id is being used to identify grub2 boot loader entries per
the Boot Loader Specification.
On systems derived from RHEL 8 and later, /etc/machine-id is being used to identify grub2 bootloader entries per
the bootloader Specification.
However, at the time of executing convert2rhel, the current machine-id can be different from the machine-id from the
time when the kernels were installed. If that happens:
- convert2rhel installs the RHEL kernel, but it's not set as default
Expand All @@ -79,18 +79,18 @@ def run(self):
"""
super(FixInvalidGrub2Entries, self).run()

loggerinst.task("Fix GRUB2 bootloader entries")
if system_info.version.major < 8:
# Applicable only on systems derived from RHEL 8 and later, and systems using GRUB2 (s390x uses zipl)
loggerinst.info("Applicable only to RHEL 8 and newer.")
return

loggerinst.info("Fixing GRUB boot loader entries.")

machine_id = utils.get_file_content("/etc/machine-id").strip()
boot_entries = glob.glob("/boot/loader/entries/*.conf")
for entry in boot_entries:
# The boot loader entries in /boot/loader/entries/<machine-id>-<kernel-version>.conf
# The bootloader entries in /boot/loader/entries/<machine-id>-<kernel-version>.conf
if machine_id not in os.path.basename(entry):
loggerinst.debug("Removing boot entry {}".format(entry))
loggerinst.debug("Removing boot entry {}.".format(entry))
os.remove(entry)

# Removing a boot entry that used to be the default makes grubby to choose a different entry as default,
Expand All @@ -99,24 +99,24 @@ def run(self):
if ret_code:
# Not setting the default entry shouldn't be a deal breaker and the reason to stop the conversions,
# grub should pick one entry in any case.
description = "Couldn't get the default GRUB2 boot loader entry:\n{}".format(output)
description = "Couldn't get the default GRUB2 bootloader entry:\n{}".format(output)
loggerinst.warning(description)
self.add_message(
level="WARNING",
id="UNABLE_TO_GET_GRUB2_BOOT_LOADER_ENTRY",
title="Unable to get the GRUB2 boot loader entry",
title="Unable to get the GRUB2 bootloader entry",
description=description,
)
return
loggerinst.debug("Setting RHEL kernel {} as the default boot loader entry.".format(output.strip()))
loggerinst.debug("Setting RHEL kernel {} as the default bootloader entry.".format(output.strip()))
output, ret_code = utils.run_subprocess(["/usr/sbin/grubby", "--set-default", output.strip()])
if ret_code:
description = "Couldn't set the default GRUB2 boot loader entry:\n{}".format(output)
description = "Couldn't set the default GRUB2 bootloader entry:\n{}".format(output)
loggerinst.warning(description)
self.add_message(
level="WARNING",
id="UNABLE_TO_SET_GRUB2_BOOT_LOADER_ENTRY",
title="Unable to set the GRUB2 boot loader entry",
title="Unable to set the GRUB2 bootloader entry",
description=description,
)

Expand All @@ -134,7 +134,7 @@ def run(self):
"""
super(FixDefaultKernel, self).run()

loggerinst.info("Checking for incorrect boot kernel")
loggerinst.task("Check for default boot kernel in /etc/sysconfig/kernel")
kernel_sys_cfg = utils.get_file_content("/etc/sysconfig/kernel")

possible_kernels = ["kernel-uek", "kernel-plus"]
Expand All @@ -143,22 +143,29 @@ def run(self):
None,
)
if kernel_to_change:
description = "Detected leftover boot kernel, changing to RHEL kernel"
loggerinst.warning(description)
diagnosis = "Detected default boot kernel {} in /etc/sysconfig/kernel.".format(kernel_to_change)
# need to change to "kernel" in rhel7 and "kernel-core" in rhel8
new_kernel_str = "DEFAULTKERNEL=" + ("kernel" if system_info.version.major == 7 else "kernel-core")
loggerinst.warning(diagnosis)
self.add_message(
level="WARNING",
id="LEFTOVER_BOOT_KERNEL_DETECTED",
title="Leftover boot kernel detected",
description=description,
id="LEFTOVER_DEFAULT_BOOT_KERNEL_DETECTED",
title="Leftover default boot kernel detected",
diagnosis=diagnosis,
description="Some systems have the default boot kernel in /etc/sysconfig/kernel set to a distribution"
" specific kernel even after such a kernel is uninstalled. We have changed it to {}.".format(
new_kernel_str
),
)
# need to change to "kernel" in rhel7 and "kernel-core" in rhel8
new_kernel_str = "DEFAULTKERNEL=" + ("kernel" if system_info.version.major == 7 else "kernel-core")

kernel_sys_cfg = kernel_sys_cfg.replace("DEFAULTKERNEL=" + kernel_to_change, new_kernel_str)
utils.store_content_to_file("/etc/sysconfig/kernel", kernel_sys_cfg)
loggerinst.info("Boot kernel {} was changed to {}".format(kernel_to_change, new_kernel_str))
loggerinst.info(
"Default boot kernel {} was changed to {} in /etc/sysconfig/kernel.".format(
kernel_to_change, new_kernel_str
)
)
else:
loggerinst.debug("Boot kernel validated.")
loggerinst.debug("The default boot kernel is correct.")


class KernelPkgsInstall(actions.Action):
Expand All @@ -169,18 +176,20 @@ def run(self):
"""Remove non-RHEL kernels."""
super(KernelPkgsInstall, self).run()

loggerinst.task("Remove non-RHEL kernels")

kernel_pkgs_to_install = self.remove_non_rhel_kernels()
if kernel_pkgs_to_install:
self.install_additional_rhel_kernel_pkgs(kernel_pkgs_to_install)

def remove_non_rhel_kernels(self):
loggerinst.info("Searching for non-RHEL kernels ...")
loggerinst.info("Searching for non-RHEL kernels.")
non_rhel_kernels = pkghandler.get_installed_pkgs_w_different_key_id(system_info.key_ids_rhel, "kernel*")
if not non_rhel_kernels:
loggerinst.info("None found.")
return None

loggerinst.info("Removing non-RHEL kernels\n")
loggerinst.info("Removing detected non-RHEL kernels.\n")
pkghandler.print_pkg_info(non_rhel_kernels)
pkgs_to_remove = [pkghandler.get_pkg_nvra(pkg) for pkg in non_rhel_kernels]
utils.remove_pkgs(pkgs_to_remove)
Expand All @@ -198,7 +207,7 @@ def install_additional_rhel_kernel_pkgs(self, additional_pkgs):
pkg_names = [p.nevra.name.replace(ol_kernel_ext, "", 1) for p in additional_pkgs]
for name in set(pkg_names):
if name != "kernel":
loggerinst.info("Installing RHEL {}".format(name))
loggerinst.info("Installing RHEL {}.".format(name))
pkgmanager.call_yum_cmd("install", args=[name])


Expand All @@ -217,4 +226,5 @@ def run(self):
"""
super(UpdateKernel, self).run()

loggerinst.task("Update RHEL kernel")
pkghandler.update_rhel_kernel()
2 changes: 1 addition & 1 deletion convert2rhel/actions/conversion/set_efi_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def run(self):
)
continue

logger.info("Moving '{}' to '{}'".format(src_file, dst_file))
logger.info("Moving '{}' to '{}'.".format(src_file, dst_file))

try:
shutil.move(src_file, dst_file)
Expand Down
25 changes: 14 additions & 11 deletions convert2rhel/actions/post_conversion/hostmetering.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ def run(self):
self.add_message(
level="WARNING",
id="INSTALL_HOST_METERING_FAILURE",
title="Failed to install host metering package.",
description="When installing host metering package an error occurred meaning we can't"
title="Failed to install the host-metering package",
description="When installing the host-metering package an error occurred meaning we can't"
" enable host metering on the system.",
diagnosis="`yum install host-metering` command returned {ret_install} with message {output}".format(
ret_install=ret_install, output=output
Expand All @@ -89,9 +89,9 @@ def run(self):
self.add_message(
level="WARNING",
id="CONFIGURE_HOST_METERING_FAILURE",
title="Failed to enable and start host metering service.",
title="Failed to enable and start the host metering service",
description="The host metering service failed to start"
" successfully and won't be able to keep track.",
" successfully and won't be able to report on the use of the system for the billing purposes.",
diagnosis="Command {command} failed with {error_message}".format(
command=command, error_message=error_message
),
Expand All @@ -109,7 +109,7 @@ def run(self):
self.set_result(
level="ERROR",
id="HOST_METERING_NOT_RUNNING",
title="Host metering service is not running.",
title="Host metering service is not running",
description="host-metering.service is not running.",
remediations="You can try to start the service manually"
" by running following command:\n"
Expand All @@ -126,7 +126,7 @@ def _check_host_metering_configuration(self):
:rtype: bool
"""
if tool_opts.configure_host_metering is None:
logger.debug("You have not enabled configuration of host metering. Skipping it.")
logger.info("You have not enabled configuration of host metering. Skipping it.")
return False

if tool_opts.configure_host_metering not in ("force", "auto"):
Expand All @@ -152,19 +152,22 @@ def _check_host_metering_configuration(self):
if tool_opts.configure_host_metering == "force":
logger.warning(
"You've set the host metering setting to 'force'."
" Please note that this option is mainly used for testing and will configure host-metering unconditionally. "
" For generic usage please use the 'auto' option."
" Note that this option is mainly used for testing and will configure host-metering unconditionally. "
" For generic usage use the 'auto' option."
)
self.add_message(
level="WARNING",
id="FORCED_CONFIGURE_HOST_METERING",
title="Configuration of host metering set to 'force'",
description="Please note that this option is mainly used for testing and"
description="Note that this option is mainly used for testing and"
" will configure host-metering unconditionally."
" For generic usage please use the 'auto' option.",
" For generic usage use the 'auto' option.",
)
elif tool_opts.configure_host_metering == "auto":
logger.debug("Automatic detection of host hyperscaler and configuration.")
logger.debug(
"Configuration of host metering set to 'auto' - host-metering will be enabled based on"
" a detected hyperscaler."
)

return True

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def run(self):
self.add_message(
level="INFO",
id="SKIPPED_MODIFIED_RPM_FILES_DIFF",
title="Skipped comparison of 'rpm -Va' output from before and after the conversion.",
title="Skipped comparison of 'rpm -Va' output from before and after the conversion",
description="Comparison of 'rpm -Va' output was not performed due to missing output "
"of the 'rpm -Va' run before the conversion.",
diagnosis="This is caused mainly by using '--no-rpm-va' argument for convert2rhel.",
Expand Down Expand Up @@ -76,8 +76,8 @@ def run(self):
self.add_message(
level="INFO",
id="FOUND_MODIFIED_RPM_FILES",
title="Modified rpm files from before and after the conversion were found.",
description="Comparison of modified rpm files from before and after " "the conversion: \n{}".format(
title="Modified rpm files from before and after the conversion were found",
description="Comparison of modified rpm files from before and after the conversion: \n{}".format(
modified_rpm_files_diff
),
)
4 changes: 2 additions & 2 deletions convert2rhel/actions/post_conversion/remove_tmp_dir.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def run(self):

try:
shutil.rmtree(self.tmp_dir)
loggerinst.info("Temporary folder {} removed".format(self.tmp_dir))
loggerinst.info("Temporary folder {} removed.".format(self.tmp_dir))
except OSError as exc:
# We want run() to be idempotent, so do nothing silently if
# the path doesn't exist.
Expand All @@ -61,6 +61,6 @@ def run(self):
self.add_message(
level="WARNING",
id="UNSUCCESSFUL_REMOVE_TMP_DIR",
title="Temporary folder {tmp_dir} wasn't removed.".format(tmp_dir=self.tmp_dir),
title="Temporary folder {tmp_dir} wasn't removed".format(tmp_dir=self.tmp_dir),
description=warning_message,
)
2 changes: 1 addition & 1 deletion convert2rhel/actions/post_conversion/update_grub.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def run(self):
level="ERROR",
id="FAILED_TO_IDENTIFY_GRUB2_BLOCK_DEVICE",
title="Failed to identify GRUB2 block device",
description="The block device could not be identified, please look at the diagnosis "
description="The block device could not be identified. Look at the diagnosis "
"for more information.",
diagnosis=str(e),
)
Expand Down
18 changes: 8 additions & 10 deletions convert2rhel/actions/pre_ponr_changes/backup_system.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ class BackupRedhatRelease(actions.Action):
id = "BACKUP_REDHAT_RELEASE"

def run(self):
"""Backup redhat release file before starting conversion process"""
logger.task("Backup Redhat Release Files")
"""Back up redhat release file before starting conversion process"""
logger.task("Back up redhat-release files")

super(BackupRedhatRelease, self).run()

Expand All @@ -79,7 +79,7 @@ class BackupRepository(actions.Action):

def run(self):
"""Backup .repo files in /etc/yum.repos.d/ so the repositories can be restored on rollback."""
logger.task("Backup Repository Files")
logger.task("Back up repository files")

super(BackupRepository, self).run()

Expand All @@ -105,7 +105,7 @@ class BackupYumVariables(actions.Action):

def run(self):
"""Backup varsdir folder in /etc/{yum,dnf}/vars so the variables can be restored on rollback."""
logger.task("Backup variables")
logger.task("Back up variables")

super(BackupYumVariables, self).run()

Expand Down Expand Up @@ -146,7 +146,7 @@ def run(self):
"""Backup changed package files"""
super(BackupPackageFiles, self).run()

logger.task("Backup package files")
logger.task("Back up package files")

package_files_changes = self._get_changed_package_files()

Expand Down Expand Up @@ -178,7 +178,6 @@ def _get_changed_package_files(self):
"""Get the output from rpm -Va command from during resolving system info
to get changes made to package files.
:return dict: Return them as a list of dict, for example:
[{"status":"S5T", "file_type":"c", "path":"/etc/yum.repos.d/CentOS-Linux-AppStream.repo"}]
"""
Expand All @@ -199,10 +198,9 @@ def _get_changed_package_files(self):
# Return empty list results in no backup of the files
return data
else:
# The file should be there
# If missing conversion is in unknown state
# The file should be there. If missing, the conversion is in an unknown state.
logger.warning("Error({}): {}".format(err.errno, err.strerror))
logger.critical("Missing file {rpm_va_output} in it's location".format(rpm_va_output=path))
logger.critical("The file {rpm_va_output} is missing.".format(rpm_va_output=path))

lines = output.strip().split("\n")
for line in lines:
Expand All @@ -220,7 +218,7 @@ def _parse_line(self, line):
if not match: # line not matching the regex
if line.strip() != "":
# Line is not empty string
logger.debug("Skipping invalid output {}".format(line))
logger.debug("Skipping invalid output: {}".format(line))
return {"status": None, "file_type": None, "path": None}

line = line.split()
Expand Down
Loading

0 comments on commit 6bd4220

Please sign in to comment.