Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1691050 - [insights plugin][rfe] update insights plugin to enable and capture information for updated insights-client rpm [NEEDINFO]
Summary: [insights plugin][rfe] update insights plugin to enable and capture informati...
Keywords:
Status: NEW
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: sos
Version: 7.6
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: rc
: ---
Assignee: Pavel Moravec
QA Contact: BaseOS QE - Apps
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-03-20 17:53 UTC by Paul Dudley
Modified: 2019-03-21 09:29 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Target Upstream Version:
pmoravec: needinfo? (pdudley)
pdudley: needinfo? (pdudley)


Attachments (Terms of Use)
example insights.py (deleted)
2019-03-20 17:53 UTC, Paul Dudley
no flags Details

Description Paul Dudley 2019-03-20 17:53:16 UTC
Created attachment 1546181 [details]
example insights.py

Description of problem:

Update insights plugin to reflect insights-client changes - see example file uploaded.

This does a few things:
 - includes insights-client rpm so the plugin is properly enabled by default 
 - includes relevant files and commands useful to troubleshoot insights issues

Comment 3 Bryn M. Reeves 2019-03-20 18:06:56 UTC
It's usually more helpful to share your suggestions as a diff against either the RHEL package source, or the current upstream master. It's much easier for maintainers to review changes when they are highlighted, than in the middle of a whole source file.

Looking at this quickly, I most of these changes are already upstream. There also appear to be several bugs in the version of the plugin attached here:

The package lists were updated already:

commit 16efa607c90c32e3a3dd7d2935ea2cc661937945
Author: Ashish Humbe <ahumbe@redhat.com>
Date:   Tue Mar 19 16:42:41 2019 +0000

    [insights] update package names and paths
    
    Resolves: #1426
    
    Signed-off-by: Ashish Humbe <ahumbe@redhat.com>
    Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

This:

	    "/var/log/insights-client",

Appears to contain log files, and so needs to be subject to the same all-logs 
This set of file:

            "/etc/insights-client/machine-id",
            "/etc/insights-client/rpm.egg",
            "/var/lib/insights"

Are not currently upstream, and can easily be added, as can this command:

        self.add_cmd_output([
	    "insights-client --support --verbose"
	])

But you don't need to pass a list to add_cmd_output() for a single item, just:

	self.add_cmd_output("insights-client --support --verbose")

is fine.

This:

    def postproc(self):
        self.do_file_sub(
            self.conf_file,
            r'(password[\t\ ]*=[\t\ ]*)(.+)',
            r'\1********'
        )

        self.do_file_sub(
            self.conf_file,
            r'(proxy[\t\ ]*=.*)(:)(.*)(@.*)',
            r'\1\2********\4'
        )

Doesn't work. The do_file_sub() method accepts a _single_ file as the first argument - you cannot pass a list here. See the current upstream plugin for an example of how to do this.

The file also contains a mix of tabs and spaces in whitespace, which is not allowed (it will cause a pycodestyle error during CI tests) - please take a look at the Contribution Guidelines for sos patch formatting and pull request details:

  https://github.com/sosreport/sos/wiki/Contribution-Guidelines

You can file an issue or a pull request upstream with these changes and we will get those included in the next available release.

Comment 4 Bryn M. Reeves 2019-03-20 18:08:07 UTC
> Appears to contain log files, and so needs to be subject to the same all-logs 

... handling as the legacy log locations already supported by the plugin.

Comment 6 Pavel Moravec 2019-03-20 19:26:12 UTC
We are quite close to devel freeze of 7.7 so deferring to 7.8.

Please provide either:
- patch/diff against current upstream (that will occur in RHEL7.7 soon)
- or a list of requirements what to change/add/enhance

Thanks Bryn for the review, that is great feedback to build on.

Comment 7 Paul Dudley 2019-03-20 19:27:15 UTC
Thanks for the link and suggestions, Bryn. I appreciate it. For this I simply edited the file already provided by sos-3.6-13.el7_6.noarch, and aside from the password not being obfuscated the result gave me the files/output we were looking for, so I submitted this anyway to get the idea out for suggestions. I should have verified against the master, and I'll do that moving forward.

Comment 8 Bryn M. Reeves 2019-03-21 09:29:53 UTC
> I should have verified against the master, and I'll do that moving forward.

No problem. It was mostly a coincidence that we happened to be merging patches for some of these changes anyway this week. Those that we get into upstream before the 7.7 rebase will automatically make the release (we can then either close this out CURRENTRELEASE, or move any remaining pieces over to 7.8).


Note You need to log in before you can comment on or make changes to this bug.