Hi again. I have rewritten the patch to have one handler which support both the old weathergoose and weathergoose 2. Unfortunately I have only had the opportunity to test it with a weathergoose 2, but it should work with the old version as well. The handler now also re-uses the old alert types for both versions of the weathergoose. In other words, the duplicates in alertmsg.conf and alert filters are removed.
_____________ Marius Halden IT-Lærling
-----Opprinnelig melding----- Fra: Morten Brekkevold [mailto:morten.brekkevold@uninett.no] Sendt: 13. mai 2011 10:48 Til: Halden Marius Kopi: nav-dev@uninett.no; John-Magne Bredal Emne: Re: Weathergoose 2 support (Bug #737446)
On Thu, 12 May 2011 16:27:04 +0200 "Halden Marius" Marius.Halden@hin.no wrote:
I have made a patch to add support for weathergoose 2, a mercurial bundle with the changes is attached to this email.
Hi Marius,
This looks great - but I do have some comments:
This patch introduces duplication of both code and alert message templates, something I would like to avoid.
The code of the new weathergoose2 plugin is identical to that of the old weathergoose plugin. Only the data structures and the MIB module have changed; I assume due to the new MIB reusing most of the object names, but rewriting trap names from the old MIB, effectively using the pattern s/TRAP$/NOTIFY/.
I would like to see the two plugins consolidated into one - or, to at least have the common code of the two extracted to a single location.
Ultimately, I am hesitant to accept the patch as-is, because it introduces a confusion as to which alerts to subscribe to in an Alert Profile. For some strange reason, the NAV alert names of the original plugin are taken directly from the corresponding trap object names of the MIB. You have copied this mis-pattern, causing each alert type in NAV to be duplicated (complete with duplicate templates in alertmsg.conf).
How does the end user know how to distinguish between a "cmClimateTempCTRAP" and a "cmClimateTempCNOTIFY" alert when setting up a filter? They both signify the same, the only difference is which version of the WeatherGoose product the user owns. If she owns multiple versions, things really start to get tricky.
At the very least, I think the new plugin should re-use the existing alert types (which calls for some code changes) - that would make the patch acceptable.
As a future improvement, though, I think the alerts should be renamed to more generic names that aren't dependent on the MIBs used.
So, to sum up my rant: Thanks for what looks to be a great contribution - if you can modify it to re-use the existing alert types from the v1 plugin I will accept it into NAV :)
-- Morten Brekkevold UNINETT