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

Provide code from repository as real Python package #20

Merged
merged 3 commits into from
Mar 12, 2022

Conversation

amotl
Copy link
Contributor

@amotl amotl commented Mar 11, 2022

Dear Frederic,

in order to improve installation convenience, we decided to add some bits to bring the repository into a shape similar to another Python-based monitoring plugin we recently worked on 1.

Along the lines of adding a corresponding setup.py file, the patch also improves the README file and splits off the Icinga configuration files into a separate folder for easier consumption and extensibility 2.

Installing the program, even without publishing the package to PyPI, is now a matter of invoking a single command 3.

pip install git+https://github.com/wernerfred/check_synology

By putting the corresponding Icinga configuration snippets into separate files, they can be easily acquired as well.

wget https://raw.githubusercontent.com/wernerfred/check_synology/master/icinga2/synology-command.conf
wget https://raw.githubusercontent.com/wernerfred/check_synology/master/icinga2/synology-services.conf
wget https://raw.githubusercontent.com/wernerfred/check_synology/master/icinga2/synology-host.conf

We hope you like those improvements.

With kind regards,
Andreas.

/cc @Tonkenfo

Footnotes

  1. https://github.com/cicerops/fritznagios

  2. The patch builds upon Use "easysnmp" and "snmpwalk" for huge efficiency gains #19, so the diff relative to this would be https://github.com/cicerops/check_synology/compare/easysnmp...package. The new README file can be directly viewed at package/README.md.

  3. This tremendously eases usability, because the interface from synology-command.conf will be more "stable" - no need to explicitly assign the Python interpreter there anymore, it will be automatically linked when expanding the console_scripts entrypoint defined in setup.py.

@amotl amotl changed the title Provide code from repository as real Python package Provide code from repository as real Python package to improve installation convenience Mar 11, 2022
@amotl amotl marked this pull request as draft March 11, 2022 21:30
@amotl amotl changed the title Provide code from repository as real Python package to improve installation convenience Provide code from repository as real Python package Mar 12, 2022
@amotl amotl force-pushed the package branch 2 times, most recently from 0e26191 to 67f9992 Compare March 12, 2022 11:33
@amotl amotl marked this pull request as ready for review March 12, 2022 11:33
setup.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved

object Host "foo.example.org" {

// Configuration section for `check_synology`
Copy link
Owner

Choose a reason for hiding this comment

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

Should we use something more randomized as IP address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure we can do that. Do you have any suggestions?

Copy link
Contributor Author

@amotl amotl Mar 12, 2022

Choose a reason for hiding this comment

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

Do you suggest it would be better to use an IP address from another private IP range, like 10.123.456.789 or 172.16.1.2? Shall we improve the inline documentation to signal to the user that she would have to put in the right value here?

Does it actually have to be an IP address? If not, I would prefer to use host names anyway. When doing so within documentation and configuration snippets, I usually use the example.org domain, which is exactly made for this purpose. So, what about synology.example.org, if that would make sense?

The benefit is that this would croak on the resolver level already, hopefully implictly telling the uninitiated user that she would have to adjust the corresponding setting.

Copy link
Contributor Author

@amotl amotl Mar 12, 2022

Choose a reason for hiding this comment

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

I've just added 885f845 to implement my suggestion. #24 will assist the user to figure things out easily when just copy/pasting configuration snippets verbatim, even without reading the documentation.

Copy link
Owner

Choose a reason for hiding this comment

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

If not, I would prefer to use host names anyway. When doing so within documentation and configuration snippets, I usually use the example.org domain, which is exactly made for this purpose. So, what about synology.example.org, if that would make sense?

Exactly my thoughts

@amotl amotl requested a review from wernerfred March 12, 2022 13:55
@wernerfred wernerfred merged commit 3d83e59 into wernerfred:master Mar 12, 2022
@wernerfred
Copy link
Owner

@all-contributors add @amotl for ideas and example

@allcontributors
Copy link
Contributor

@wernerfred

I've put up a pull request to add @amotl! 🎉

@amotl amotl deleted the package branch March 13, 2022 14:02
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.

None yet

2 participants