Supporting files for APCUPSD Addon #101
Conversation
There was a problem hiding this comment.
This file will be populated with an apcupsd.conf by the cli if the user enables running apcupsd in a container. This file is present in the installer so that it has the correct permissions to be overwritten by the cli.
Please let me know if there is a preffered file path to use here.
There was a problem hiding this comment.
I've moved this to install/addons/apcupsd/addon_apcupsd.conf - all of the addon-specific files will go into that folder and get mounted as a volume in your container.
There was a problem hiding this comment.
Because this addon may optionally run 1 or 2 containers depending on it's configuration I have commented out the services: definition.
In the scenario where a user does not want to run apcupsd in a container, the addon_apcupsd.network.tmpl template will be used which does not define the service "addon_apcupsd". If these files were uncommented, the cli would complain that "addon_apcupsd" does not have an image / build context specified.
Let me know if there is a more elegant solution here.
There was a problem hiding this comment.
The best way to do this is one template file per container, so I'll break it out into two separate files.
There was a problem hiding this comment.
Cool, I want to flag that you'll want to test that the service sucessfully restarts when toggling between "Container" and "Network" mode. Keep a look out for a bug where the service tries and fails to start the apcupsd container even though we don't need this container because we are running in network mode. This happened due to the presence of an (empty) service definition inside the overrides folder.
There was a problem hiding this comment.
NOTE: This apcupsd job is added to the prometheus config regardless of whether the user has the addon enabled. I don't think this is an issue as it fails to fetch the metrics silently, but perhaps it is additional overhead we would not want to add? Let me know if there is a method to conditionally add items to this configuration file based on an environment variable.
There was a problem hiding this comment.
I think failing silently in this case is fine, we already do it (geth and eth1 are mutually exclusive, as the later is used for Besu and Nethermind). I'll put a comment in there for posterity.
danimbrogno
changed the title
There was a problem hiding this comment.
This doesn't immediately look like a template to me, it just looks like a static file that may or may not be deployed. Template files in this dir are usually Docker container definitions - we may find a better place for this to live.
There was a problem hiding this comment.
Hey that's a good point. I was thinking about letting the user customize this file via the TUI but that wasn't in the original scope of the grant and I've spent more time on this than I originally anticipated.
That's why it is in the template folder. If we move it out would it be a pain to put it back in here later if I wanted to build out this feature?
This PR contains changes to the smartnode installer to support the APCUPSD addon proposed in GA022303.
When a user has metrics enabled, this addon allows a node operator to:
This PR must be accepted alongside the PR to the smartnode repository.