-
Notifications
You must be signed in to change notification settings - Fork 91
Add --category option #1012
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
base: develop
Are you sure you want to change the base?
Add --category option #1012
Conversation
Hi @g-bougard , do you want deeper integration (installers + configs + per module check) too ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @0xff8,
I'm still perplex.
I see few problem with this option. It can break inventory if miss-used.
If I merge it, I'll modify few things to force required category. For example, bios and hardware categories are required for glpi and os is required if software category is enabled (see
glpi-agent/lib/GLPI/Agent/Task/Inventory.pm
Line 235 in 75a8699
$keep{hardware} = 1; |
A problem is still this option could be very long as the default would be: accesslog,antivirus,battery,bios,controller,cpu,database,drive,environment,firewall,hardware,input,licenseinfo,local_group,local_user,lvm,memory,modem,monitor,network,os,port,printer,process,provider,psu,registry,remote_mgmt,rudder,slot,software,sound,storage,usb,user,video,virtualmachine
Maybe another way would be to use a categories
option which would support some keywords with preselected category values: full
, system
, basic
, extended
or what ever people can think of.
no strict 'refs'; ## no critic (ProhibitNoStrict) | ||
my $category = &{$module."::category"}(); | ||
if ($category && $self->{disabled}->{$category}) { | ||
if ($category && ($self->{disabled}->{$category} || ( %{ $self->{enabled} } && !exists $self->{enabled}->{$category} ))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I would prefer a elsif
after the original case with a log telling category is "not enabled" in place of "disabled". This way, we can better understand why a module remains disabled while reading the log.
datadir => $self->{datadir}, | ||
inventory => $self->{inventory}, | ||
no_category => $self->{disabled}, | ||
category => $self->{enabled}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed, no module uses category
param in their isEnabled
method.
thank you very much for the review @g-bougard ! My initial goal was not to change the existing behavior. So, if no
By default, the plan is to request only the privacy-friendly data, i.e. As an example, currently, the command we use looks like: I am happy if you share your vision about |
Co-authored-by: Guillaume Bougard <gbougard@teclib.com>
I haven't finished yet, sorry, I found after merging your changes, not all reports are working properly. Please, let me spent a bit more time on this . |
} | ||
# Software category requires os to be enabled too | ||
if ($self->{enabled}->{software} && !$self->{enabled}->{os}) { | ||
$self->{logger}->debug("Forcing os category as required by software one") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies, one char is missing at the end here:
$self->{logger}->debug("Forcing os category as required by software one") | |
$self->{logger}->debug("Forcing os category as required by software one"); |
Here is an initial proposal for a new
--category
option, that if present - will enable only desired metrics and exclude everything else.As continuation of #1002