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.

Sign up for GitHub

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

Jump to bottom

Bugfix #86

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Bugfix #86

wants to merge 5 commits into from

Conversation

mbe-financial-com
Copy link

Hi,

I spent a few hours hacking on this check, resulting in the four commits here -- hope they're useful.

  • "fix option parsing: more concise, check logic": you'll probably want this one, it fixes Use of uninitialized value $strOption in lc #78. The GetOptions() was unnecessarily verbose, and there was a logic error in the option checking, leading to the "Use of uninitialized value $strOption" warning.
  • "fix a few logic/undef-var errors": perhaps these checks even worked accidentally but the parentheses were definitely not how they should be: defined($s && $s eq "foo") instead of defined($s) and $s eq "foo". Even where they were, in this function I changed && to the unambiguously low-precedence logical and. Guess you'll want this, too.
  • "reduce boilerplate": Subrefs to the rescue! This is a pretty big change, eliminating the whole huge if-elsif construct that checks $strOption. Now it errors out if the option is unknown, and does so in just over a quarter of the lines. I didn't bother with the Hungarian notation so I totally understand if you don't want this, or want to re-style it.
  • "eliminate repetitive child_add() calls": TBH this is completely untested in real life as we don't use that feature, but at least it gives the same results as before. It introduces two little helper functions to eliminate a whole bunch of function calls occurring in almost every get_* function. May be useful, maybe not.

I thought of adding support for Monitoring::Plugin as it would probably cut the LOC count in half and eliminate all the tedious status var juggling, but that would be too much for now ;)

@mbe-financial-com
Copy link
Author

Sorry for the stupid title, I had to reload the page and it reset to my nondescript branch name ...

@willemdh
Copy link
Collaborator

willemdh commented May 5, 2020

Thanks @mbe-financial-com ! Looking good, we will try to test it asap. Could take a few weeks.

@willemdh willemdh mentioned this pull request May 6, 2020
Merged
@willemdh
Copy link
Collaborator

@mbe-financial-com Sorry, but I can't merge this. We get the following error when running your version:

/usr/local/nagios/libexec/check_netapp_ontap9a.pl -H <netapphost> -u nagios -p <password> -o aggregate_health -c "failed, offline"
Unknown warnings category 'experimental::smartmatch' at /usr/local/nagios/libexec/check_netapp_ontap9a.pl line 28.
BEGIN failed--compilation aborted at /usr/local/nagios/libexec/check_netapp_ontap9a.pl line 28.

@mbe-financial-com
Copy link
Author

@willemdh But that line is from your version, introduced in 255e9b9!? I didn't change anything about that. Is that a very old Perl you're testing on? It should fail with the original HEAD, too.
But in any case, I'd say just drop the smartmatch. It's only used in three instances to do something that's more efficiently done with a hash anyway, and it has been deprecated in recent Perl versions because it wasn't a good feature to begin with. I'll make another commit.

Elias481 added a commit to Elias481/check_netapp_ontap that referenced this pull request Jul 20, 2020
@Elias481
spare disks in shared mode, optional aggregte spare-count spares clus…
cd21084 
…ter- or node wide, fix filter to query only disks from home-node

* add handling of spare disks in "shared" mode (partitioned or formated disks)
  these are logically not assigned to a specific nodes spare disk capacity so are shown at a _SHARED_ node and not shown in single-node mode
* add a suboption "aggregate" that allows to sum up/aggregate all disks in the cluster (if run against a specific "node" the "shared" mode disks are accounted for the node)
* fix query-building to find only disks for the specific home-node (the error was not visible before because it was post-filtered in result function)
* fix broken branching conditions within these function (see also district09#86)
@willemdh
Copy link
Collaborator

@mbe-financial-com Sorry it took me so long to test this. But it seems even with your latest commit, it still fails...

image

@willemdh
Copy link
Collaborator

@mbe-financial-com Do you still want to try resolve this, if so please resolve conflicts. Tx

@willemdh
Copy link
Collaborator

@mbe-financial-com Another pr merged, see #89

Elias481 added a commit to Elias481/check_netapp_ontap that referenced this pull request Sep 24, 2020
@Elias481
spare disks in shared mode, optional aggregte spare-count spares clus…
f0f011c 
…ter- or node wide, fix filter to query only disks from home-node

* add handling of spare disks in "shared" mode (partitioned or formated disks)
  these are logically not assigned to a specific nodes spare disk capacity so are shown at a _SHARED_ node and not shown in single-node mode
* add a suboption "aggregate" that allows to sum up/aggregate all disks in the cluster (if run against a specific "node" the "shared" mode disks are accounted for the node)
* fix query-building to find only disks for the specific home-node (the error was not visible before because it was post-filtered in result function)
* fix broken branching conditions within these function (see also district09#86)
@mbe-financial-com
Copy link
Author

Did you test the version with the conflict marker?! Because that's the only way I could imagine how to get a compilation error on line 28. My version completely eliminates the whole smartmatch magic so the no if ... hackery isn't needed any more. I rebased my PR on the latest master so it includes the fixes from #87.

@willemdh
Copy link
Collaborator

willemdh commented Sep 25, 2020 •
edited
Loading

@mbe-financial-com I copied your version to my pr server and launched some checks, which failed with the error in the screenshot. As your pr failed 3 times yet, the moment I find some spare time I will first test and merge #90
I hope u understand.

@willemdh
Copy link
Collaborator

@mbe-financial-com Hello, Just merged @Elias481 's PR. Can you rebase pls? Thanks.

@willemdh
Copy link
Collaborator

@mbe-financial-com Merged #91 rebase required if you still watn to see this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use of uninitialized value $strOption in lc
2 participants
@mbe-financial-com @willemdh