On Thu, March 10, 2016 23:22, Vadim Zhukov wrote:
> 2016-03-09 17:08 GMT+03:00 Kirill Bychkov <ki...@linklevel.net>:
>> On Thu, March 3, 2016 16:57, Kirill Bychkov wrote:
>>> Him guys!
>>> This is a port of PNP4Nagios, an addon for Nafios and Oconga for analyzing
>>> performance data and storing it in RRD.
>>> Current port is partially based on an old one from henning@ [1] and tested
>>> for more than a month with Icinga 1.x processing data from about 400 hosts.
>>> It could be splitted to Nagios and Icinga 2.x flavors if there are some
>>> interest in them and one can test it with.
>>>
>>> [1] http://marc.info/?l=openbsd-ports&m=140803165912579&w=2
>>>
>>> Comments? OKs?
>> Objections? :)
>>
>> Updated tarball with fixed typos.
>
> Here are some more nits:
>
> ... in Makefile:
>
>> INSTALL_OPTS="-o roog -g bin" \
Damn, as usual... :)
>
> One more tyop. ;)
>
>> SYSCONFDIR = ${BASESYSCONFDIR}/pnp4nagios/
>> LOCALSTATEDIR = ${BASELOCALSTATEDIR}/pnp4nagios
>
> Why slash is added in one case but not in the other?
Just a paste effect. Should not change anything, byt a style matters, yes.
>
>
>> # fix broken symlink in tarball
>
> If this symlink gets packaged, then it should be relative one, like:
>
> ln -sf ../en_US/dwnld.html \
> ${WRKSRC}/share/pnp/documents/de_DE/dwnld.html
At a fake stage a file is installed, not symlink. Whithout this trick fake is
broken because upstream foret to symlink it while creating tarball.
>
> ... in patches:
>
> At least patch-sample-config_httpd_conf_in needs justification, why it's
> needed.
>
> ... in pkg/README-cgi:
>
>> Apache2 configuration for PNP4Nagios is stored under:
>> /var/conf/modules.sample/pnp4nagios.conf
Yes, mi bad. This should be /var/www
>
> /var/conf? I suppose this should be ${LOCALSTATEDIR}/apache2/conf, or
> something like that.
Bit not LOCALSTATEDIR which is set to /var/pnp4nagios.
>
> ... in pkg/PLIST-main:
>
> Is it intended that not all share/example/* stuff has its @sample
> counterpart under ${SYSCONFDIR}?
I've aded only mandatory configs. Others are not in my setup but probably
would be useful for others.
>
>> share/doc/pkg-readmes/${FULLPKGNAME}
>> @owner _icinga
>> @sample ${LOCALSTATEDIR}/
>> @sample ${LOCALSTATEDIR}/stats/
>> @sample /var/log/pnp4nagios.log
>
> I'm not sure the latter is supposed to work, or won't fail at some
> point in the future. @sample, when applied to a file, mean "take the
> file from the line above and copy it here". And you have a non-files
> line above up to README one. If you want to create an empty file owned
> by someone, you may either use something like:
Great catch. It is a file. It's a readme (it is above in the list, before all
this directories you've mentionrd). I should create a "dumb" one first and
then @sample it.
>
> @exec-add test -e /var/log/pnp4nagios.log || install -o _nagios -g
> _nagios -m 0640 /dev/null /var/log/pnp4nagios.log
> @extraunexec test -s /var/log/pnp4nagios || rm -f /var/log/pnp4nagios.log
>
> or use /var/log/pnp4nagios/ directory instead, owned by _nagios, where
> application could create/rename/remove files without problem. I'd
> recommend go the 2nd way, as we usually do.
Well, I'm not sure that a subdirectory is really needed for the one log file,
but it is a real alternative. Or just sampling an empty file...
I'll make new tarball tomorrow.
>
> --
> WBR,
> Vadim Zhukov
>
