Hi!
I have made a patch to add support for weathergoose 2, a mercurial bundle with the changes is attached to this email.
Regards
Marius Halden
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 :)
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
On Wed, 18 May 2011 14:40:32 +0200 "Halden Marius" Marius.Halden@hin.no wrote:
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.
Thanks! Not sure that the code itself was an improvement (but then, the old code needed some cleanup as well).
Anyway, your patches prompted me to do some tinkering of my own. Based on your original patch, I wrote these changes:
https://bitbucket.org/mbrekkevold/nav-wxgoose2/changesets
I've added some unit tests for this code as well (run them using `make check` in the python dir), but I'm unable to do a live test, as it seems our own WeatherGoose 1 is not functioning at the moment.
Could you test this code on your WeatherGoose 2?
(There also seems to be lots of other tiny issues with the original plugin code that I haven't addressed).
I have tested your update and it seems to work fine with WeatherGoose 2.
_________________ Mvh. Marius Halden IT-Lærling
-----Opprinnelig melding----- Fra: Morten Brekkevold [mailto:morten.brekkevold@uninett.no] Sendt: 24. mai 2011 14:49 Til: Halden Marius Kopi: nav-dev@uninett.no Emne: Re: SV: Weathergoose 2 support (Bug #737446)
On Wed, 18 May 2011 14:40:32 +0200 "Halden Marius" Marius.Halden@hin.no wrote:
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.
Thanks! Not sure that the code itself was an improvement (but then, the old code needed some cleanup as well).
Anyway, your patches prompted me to do some tinkering of my own. Based on your original patch, I wrote these changes:
https://bitbucket.org/mbrekkevold/nav-wxgoose2/changesets
I've added some unit tests for this code as well (run them using `make check` in the python dir), but I'm unable to do a live test, as it seems our own WeatherGoose 1 is not functioning at the moment.
Could you test this code on your WeatherGoose 2?
(There also seems to be lots of other tiny issues with the original plugin code that I haven't addressed).
-- Morten Brekkevold UNINETT
On Tue, 24 May 2011 16:01:42 +0200 "Halden Marius" Marius.Halden@hin.no wrote:
I have tested your update and it seems to work fine with WeatherGoose 2.
Great :) We just got our WeatherGoose 1 back online, so I'll be testing on it shortly. If all is successful, we can get this in to the final 3.9.0 versioan.
-
Halden Marius -
Morten Brekkevold