Skip to content

[report] Fix formatting of distros in plugins_overview #4071

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

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

Conversation

jcastill
Copy link
Member

@jcastill jcastill commented Jul 8, 2025

The formatting of the distros column was leaving the string 'Plu' when it found 'PluginOpt'. This fix
explicitly deals with both 'PluginOpt' and 'Plugin' strings and removes them.


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname [email protected]?
  • Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
  • Are all passwords or private data gathered by this PR obfuscated?

Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-4071
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@arif-ali
Copy link
Member

arif-ali commented Jul 8, 2025

I never used the plugins_overview in the past, so looking into it, there's probably other fixes we ought to do as well, as there are other issues too

If the sos.report.plugins import is the only line, and then there is a comment/variables before the class deceleration, then we'll see the whole comment.

  • authd
  • kubernetes
  • charmed_postgresql
  • openstacK_instack
  • openstack_mistral
  • origin
  • validation_framework
  • vdsm

Either we update these plugins so that we don't have that, or we fix the plugins_overview.py; Personally, I wpuld lean towards fixing the plugins_overview.py

thoughts?

@arif-ali arif-ali added the Status/Needs Review This issue still needs a review from project members label Jul 8, 2025
@jcastill
Copy link
Member Author

jcastill commented Jul 9, 2025

I agree, I think fixing plugins_overview.py may be the way to go.

The formatting of the distros column was leaving the
string 'Plu' when it found 'PluginOpt'. This fix
explicitly deals with both 'PluginOpt' and 'Plugin'
strings and removes them.

Signed-off-by: Jose Castillo <[email protected]>
@jcastill jcastill force-pushed the jcastillo-plugin_overview_plugin_column branch from 41c2aca to 32602fe Compare July 9, 2025 09:17
@jcastill
Copy link
Member Author

jcastill commented Jul 9, 2025

This version should address the issues you found in the other plugins @arif-ali

@arif-ali
Copy link
Member

arif-ali commented Jul 9, 2025

I just found one more scenario

from sos.report.plugins import Plugin, RedHatPlugin, \
    DebianPlugin, UbuntuPlugin, CosPlugin

You then get the following in the distros list

"\    Debian"

I found 2 scenarios of this, apologies I didn't find this one earlier

@jcastill
Copy link
Member Author

jcastill commented Jul 9, 2025

No apologies needed at all @arif-ali , and thank you for taking the time to check this.
Somehow I didn't catch these either in the csv and json outputs. I'll work on this tomorrow.

@jcastill
Copy link
Member Author

@arif-ali I was thinking... it may be better to change the way we import in these two places (I've found kdump and release plugins, let me know if you found others) so it's consistent with the other plugins' imports of using parenthesis for multi-line imports?
I'll open a new PR for that, but feel free to close it if you think it makes no sense.

jcastill added a commit to jcastill/sos that referenced this pull request Jul 10, 2025
Change the format of the import line for plugins
to parenthesis so it matches what we do in all
other plugins that contain multiline imports.
Also, this change ensures that the script
plugins_overview.py's output is consistent
across all plugins and doesn't contain
errors.

Related: sosreport#4071

Signed-off-by: Jose Castillo <[email protected]>
TurboTurtle pushed a commit that referenced this pull request Jul 10, 2025
Change the format of the import line for plugins
to parenthesis so it matches what we do in all
other plugins that contain multiline imports.
Also, this change ensures that the script
plugins_overview.py's output is consistent
across all plugins and doesn't contain
errors.

Related: #4071

Signed-off-by: Jose Castillo <[email protected]>
Copy link
Member

@arif-ali arif-ali left a comment

Choose a reason for hiding this comment

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

now that the other PR is done, this should be all good now

@arif-ali arif-ali added the Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer label Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer Status/Needs Review This issue still needs a review from project members
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants