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.

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

[RHELC-1761] Format messages for consistency #1438

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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.")
Copy link
Member

Choose a reason for hiding this comment

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

This will be unique as most don't have a dot in them, but definitely more valid with a dot than without. Just an FYI

Copy link
Member Author

Choose a reason for hiding this comment

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

Most info messages had a full stop. For consistency I've added the full stop to all of them (and to other levels such as debug or critical as well).

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
60 changes: 35 additions & 25 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("Skipped. Only relevant 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,
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
Copy link
Member

Choose a reason for hiding this comment

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

Dots seems to be removed by accident?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you're referring to exactly here. In the line above I changed the debug log level to info because without the --debug option users wouldn't see this message that is more than just of debugging nature. No dots removed.


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",
Copy link
Member

Choose a reason for hiding this comment

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

Dot removed here as well

Copy link
Member Author

Choose a reason for hiding this comment

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

This is intentional. It's one of the goals of the PR - to remove full stops from message titles for consistency. Most titles didn't have it, so I opted for removing them from all.

The message titles are printed this way:

(INFO) MODIFIED_RPM_FILES_DIFF::FOUND_MODIFIED_RPM_FILES - Modified rpm files from before and after the conversion were found
     Description: ...

In Insights they are displayed in this fashion:
Screenshot from 2024-09-03 17-20-22

In both cases (CLI, Insights) it's more suitable to have the titles without full stops.

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")

Comment on lines +54 to 56
Copy link
Member

Choose a reason for hiding this comment

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

I would say Backup instead of back up. What is the Red Hat preferred word?

Copy link
Member Author

Choose a reason for hiding this comment

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

Backup is a noun, back up is a verb.
"You are backing up a file and then you have a file backup."

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
Loading