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.

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

Add pfSense-pkg-node_exporter #653

Closed
wants to merge 4 commits into from
Closed

Add pfSense-pkg-node_exporter #653

wants to merge 4 commits into from

Conversation

carlpett
Copy link
Contributor

This PR adds a pfSense package for node_exporter. It also bumps the existing node_exporter port version.
Tested on 2.4.4-r3.

Note to the reviewer: I've never really touched FreeBSD itself outside of pfSense before, as well as not having any previous experience with the pfSense package, so probably makes sense to check for silly mistakes or misunderstandings.

Signed-off-by: Calle Pettersson <calle@cape.nu>
@carlpett
Copy link
Contributor Author

carlpett commented Jul 6, 2019

@jim-p Sorry for direct ping, but it is not clear to me if I missed some step here to trigger someone to take a look, or if it just summertime and things are moving slow?

@carlpett
Copy link
Contributor Author

carlpett commented Aug 7, 2019

@jim-p Ping?

@jim-p jim-p requested a review from rbgarga August 7, 2019 12:16
@jim-p
Copy link
Contributor

jim-p commented Aug 7, 2019

You didn't miss anything, as soon as we have some spare cycles to review it, we'll review it. Thanks!

sysutils/node_exporter/Makefile Outdated Show resolved Hide resolved
sysutils/pfSense-pkg-node_exporter/Makefile Outdated Show resolved Hide resolved
sysutils/pfSense-pkg-node_exporter/Makefile Outdated Show resolved Hide resolved
@carlpett
Copy link
Contributor Author

carlpett commented Aug 8, 2019

I've pushed fixes for the comments, thanks for taking a look @rbgarga!

@carlpett
Copy link
Contributor Author

@rbgarga @jim-p Any spare cycles coming up?

@jim-p jim-p requested a review from rbgarga December 12, 2019 20:47
@carlpett
Copy link
Contributor Author

Thanks @jim-p! Updated as per your suggestions


do-install:
${MKDIR} ${STAGEDIR}${PREFIX}/pkg
${MKDIR} ${STAGEDIR}${PREFIX}/etc/rc.conf.d
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No files are going to be installed here, no need to create this directory.

${MKDIR} ${STAGEDIR}${DATADIR}
${INSTALL_DATA} ${FILESDIR}${PREFIX}/pkg/node_exporter.xml ${STAGEDIR}${PREFIX}/pkg
${INSTALL_DATA} ${FILESDIR}${PREFIX}/pkg/node_exporter.inc ${STAGEDIR}${PREFIX}/pkg
${INSTALL_DATA} ${FILESDIR}${PREFIX}/etc/rc.conf.d/node_exporter ${STAGEDIR}${PREFIX}/etc/rc.conf.d/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file doesn't exist, do not try to install it

@@ -0,0 +1,68 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file seems to be a copy of sysutils/node_exporter init script, we don't need to carry it here

${INSTALL_DATA} ${FILESDIR}${PREFIX}/pkg/node_exporter.inc ${STAGEDIR}${PREFIX}/pkg
${INSTALL_DATA} ${FILESDIR}${PREFIX}/etc/rc.conf.d/node_exporter ${STAGEDIR}${PREFIX}/etc/rc.conf.d/
${INSTALL_DATA} ${FILESDIR}${DATADIR}/info.xml ${STAGEDIR}${DATADIR}
${INSTALL_SCRIPT} ${FILESDIR}${DATADIR}/node_exporter.sh ${STAGEDIR}${DATADIR}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this file is really a copy of sysutils/node_exporter init script, we don't need to carry it here and it should be removed

${INSTALL_DATA} ${FILESDIR}${PREFIX}/etc/rc.conf.d/node_exporter ${STAGEDIR}${PREFIX}/etc/rc.conf.d/
${INSTALL_DATA} ${FILESDIR}${DATADIR}/info.xml ${STAGEDIR}${DATADIR}
${INSTALL_SCRIPT} ${FILESDIR}${DATADIR}/node_exporter.sh ${STAGEDIR}${DATADIR}
${INSTALL_SCRIPT} ${FILESDIR}${DATADIR}/interface-collector.py ${STAGEDIR}${DATADIR}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this package is installing a python script, we need to have USES= python to make sure python is installed as a dependency. If it requires any extra python module available on ports, it also should be added to RUN_DEPENDS

@carlpett
Copy link
Contributor Author

Thanks @rbgarga! Pushed fixes now

@rbgarga
Copy link
Member

rbgarga commented Dec 17, 2019

I've manually merged it to incorporate some ports specific changes.

Also opened a redmine ticket to track it - https://redmine.pfsense.org/issues/9974. Thanks!

@rbgarga rbgarga closed this Dec 17, 2019
@carlpett carlpett deleted the node_exporter branch June 28, 2020 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants