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

Added ability to specify a directory of config files #64

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

omnistat
Copy link
Contributor

After using this plugin for a little while, it started to become burdensome to navigate one huge configuration file for every metric of multiple modbus devices. I read through some issues and saw others mention this as well.

I decided to add basic functionality to specify either a single configuration file or a directory of configuration files in the format of modbus.yml. This will load all configurations as if they were in one file, but allows users to seperate them out for readability.

The folder name must end with .d to distinguish between a file or folder. For code simplicity, all files within the directory will attempt to be loaded, including non-yaml files, and error accordingly.

Example:

modbus_configs.d
├── module1.yml
└── module2.yml

./modbus_exporter --config="modbus_configs.d"

Reflecting this change, the --config.file parameter has been changed to --config to be more general.

@omnistat omnistat changed the title Added ability so specify a directory of config files Added ability to specify a directory of config files Feb 20, 2024
Copy link
Collaborator

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

This is not the way to handle this. Take a look how the multiple files/glob handling was done in the snmp_exporter.

prometheus/snmp_exporter#970

@omnistat
Copy link
Contributor Author

Thanks for the correction. I'll work on implementing this method instead. I wasn't aware this existed with the snmp exporter but I can see the rationale.

@omnistat omnistat marked this pull request as draft February 20, 2024 11:06
@omnistat omnistat force-pushed the main branch 5 times, most recently from d07bc55 to 6a7b0b7 Compare February 22, 2024 12:50
@omnistat omnistat marked this pull request as ready for review February 22, 2024 20:06
@omnistat omnistat requested a review from SuperQ February 22, 2024 20:16
@omnistat
Copy link
Contributor Author

How is this? It was quite simple to implement and functions very similarly to snmp_exporter. I'm not sure if the change warrants an additional test case.

Copy link
Collaborator

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@SuperQ SuperQ requested a review from RichiH February 22, 2024 21:19
@bastischubert
Copy link
Collaborator

@omnistat could you please update the readme.md that has a conflict, so i can merge this pr ;)

@omnistat
Copy link
Contributor Author

@bastischubert Done :)

@bastischubert bastischubert merged commit f42ee2c into RichiH:main Mar 11, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants