lukecyca / pyzabbix

Python Zabbix API
970 stars 313 forks source link

Unify Zabbix python librairies #197

Open jooola opened 2 years ago

jooola commented 2 years ago

In order to fix #164, I though we could reach feature parity with https://github.com/adubkov/py-zabbix and unify the work effort into a single project. This would have allowed us to fix this namespace issue.

@szuro reached out and proposed to merge his own projects instead of https://github.com/adubkov/py-zabbix. See discussion at https://github.com/lukecyca/pyzabbix/pull/194#issuecomment-1199444876

The plan would be to merge https://github.com/szuro/zabbix-enums and https://gitlab.com/szuro/zappix into this project.

I don't have much time to put into merging all the projects, but I can do some work once we planned everything.

Before all, we should agree on the development process:

Merge zabbix enums in the api namespace

I think we can start by merging the enum library in the project, as the changes does not seem to require much thinking apart from naming.

I wonder if we want to keep the enums namespace, or have a more generic namespace such as types or typing ?

We could have the following structure:

from pyzabbix import ZabbixAPI
# or
from pyzabbix.api import ZabbixAPI

# And for types
from pyzabbix.api.types.z60 import SomeEnum
from pyzabbix.api.typing.z60 import SomeEnum

@szuro Are there any breaking changes you intend to do will the enums library is merged ? Is this library automatically generated ? Is there a way to generate it ?

Merge zappix in the pyzabbix namespace

First, apart from fixing #164, what does this merge bring/fix ? Is there any way we could fix this without having to merge the projects ? I prefer to ask, so we don't dive into a whole lot of work without a well defined goal.

Maybe both libraries should stay separated but each can be maintained under the same umbrella, advertising/linking to the other project for missing features.

That being said, we might bloat the project with dependencies while some people might only want to use the API client. So I was thinking, maybe we want to hide the added features behind a extra flag such as pyzabbix[sender] ?

Regarding the proposed namespace in https://github.com/lukecyca/pyzabbix/pull/194#issuecomment-1205503069 wrapping everything in a network namespace seem overkill, maybe we simply want to have pyzabbix.sender and pyzabbix.agent (I am probably missing some packages ?) We can always have a shared private namespace for network related code in pyzabbix._network for shared protocols/socket/...

How could we split the merge into smaller parts ? Can the agent and sender be merged independently ?

I probably missed a few points.

cc @szuro

linceaerian commented 2 years ago

About First, apart from fixing https://github.com/lukecyca/pyzabbix/issues/164, what does this merge bring ?, if you bring the native sender protocol from szuro/zappix the main thing that I see will miss is the use_config parameter, _load_from_config from https://github.com/adubkov/py-zabbix.

It allow to parse the agent config file and directly extract the data needed to send package. While it's not perfect, it simplify the arguments to provide to the script and keep the function in case of change in zabbix_agent configuration.

jooola commented 2 years ago

About First, apart from fixing https://github.com/lukecyca/pyzabbix/issues/164, what does this merge bring ?, if you bring the native sender protocol from szuro/zappix the main thing that I see will miss is the use_config parameter, _load_from_config from https://github.com/adubkov/py-zabbix.

It allow to parse the agent config file and directly extract the data needed to send package. While it's not perfect, it simplify the arguments to provide to the script and keep the function in case of change in zabbix_agent configuration.

Such helper function can naturally be added, but I personally prefer not to embed this in the Sender class.

Maybe something like:

from pyzabbix import Sender, parse_zabbix_servers_from_config

sender = Sender(servers=parse_zabbix_servers_from_config())
sender = Sender(servers=parse_zabbix_servers_from_config("my/custom/path.conf"))

Though, I wonder how to handle the new HA notation (in zabbix >= 6.0) for picking the right server.

linceaerian commented 2 years ago

Good question, it's a tad hard to immerse in the C code from the agent...

Maybe those part would be relevant to the question:

I would tend to think you cannot select the "right" server and either try to send to each one ensuring you should have at least one in each "cluster" that works, but I'm not fully "sure" per se.

And yes it does not need to be necessarily embed inside the Sender, It could be even more abstract, something like

from pyzabbix import Sender, Config
config = Config(file="my/custom/path.conf")
sender = Sender(servers=config.servers_active)

So you can reuse the Config module if you need it for another module ? Ex: local requests with pyzabbix.agent (to get PSK configuration or other).

jooola commented 2 years ago

So you can reuse the Config module if you need it for another module ? Ex: local requests with pyzabbix.agent (to get PSK configuration or other).

I like this idea of parsing extra details from the config file. I think @szuro has a better idea what else could be queried from there.

szuro commented 2 years ago

Do we agree to follow conventional commits spec ? Should we follow semantic versioning ? Maybe that's obvious, but we should be sure we all use the same versioning schema. How should we document the libraries ? I am in favor of picking a docstring format (maybe ?) and generate the documentation from the docstrings.

  1. yes
  2. maybe? for zabbix_enums I'm putting the latest supported Zabbix version in package version, so users know what to expect
  3. I'm all for docstrings

Are there any breaking changes you intend to do will the enums library is merged ? Is this library automatically generated ? Is there a way to generate it ?

I'll probably ditch import * in __init__.py in every version. This means you won't be able to from zabbix_enums.z60 import * and have all enums in the namespace, but you'd have to from zabbix_enums.z60.host import * or similar. This should be better for users in the long run, and will solve conflicting enum names.

The lib is updated by hand, but i have a simple Zabbix doc differ, so I know what changed between versions.

I wonder if we want to keep the enums namespace, or have a more generic namespace such as types or typing ? I think that enums would be best. They are not exactly types.

Ad use_config functionality: it is planned.

I originally didn't want to embed it in the Sender class, but instead make a Parent class for both Sender and Agent, as both of them have a use for the config file.

What can be extracted:

So a bunch of things really.

from pyzabbix import Sender, Config
config = Config(file="my/custom/path.conf")
sender = Sender(servers=config.servers_active)

Maybe this would work better:

from pyzabbix import Sender, Config
config = Config(file="my/custom/path.conf")
sender = Sender.from_config(config)

As to electing the Active Zabbix server. My original idea was to provide a list of IP addresses to Sender.__init__ and try them until a proper response is received. Save the address and use it until an error occurs. Then start over.

jooola commented 2 years ago

@lukecyca Could you give write access to @szuro please ?

paalbra commented 1 year ago

My two cents:

Personally I've been using this project only to interface with the API, which is what it is supposed to do: https://github.com/lukecyca/pyzabbix/blob/511e3a3c473970acc84b706cfa5617e7e05c2fa0/README.md?plain=1#L3

I like that the project is simple and only does this.

Adding other features like zabbix-sender seems out of scope for me. I'm sure other projects could do that just fine?

I'd like this project to continue to be an API interface, and an API interface only. So I'd suggest not expanding this current scope.

As I final note I'd like to quote from the unix philosophy:

Make each program do one thing well. To do a new job, build afresh rather than complicate old programs by adding new "features".

I feel that this is what this project currently does very well. Merging other features will just add unnecessary complexity.

szuro commented 1 year ago

So, it's been a while since this started. Requests for zabbix sender are still ever present, so it would be nice to at least inform the users of concrete future plans.

I don't think we should merge anything in the heat of the moment just to break it later...

jooola commented 1 year ago

Hey @szuro, Yes, it has been a while.

My understanding is that people are looking for a zabbix sender, and only find the project at https://github.com/adubkov/py-zabbix. The adubkov/py-zabbix project haven't seen any release/changes merged since 2020, and one might assume it is dead.

Until we find a magical solution, I think we should add a link to the zappix project on Gitlab for people looking for a sender.

I do agree that we might want to follow the Unix philosophy, and make sure both projects (https://gitlab.com/szuro/zappix and https://github.com/lukecyca/pyzabbix) play nice with each other.

In any case, if we were to merge more code in this project, I would need the authors of this code to help me maintain it. But I haven't heard from @lukecyca about granting @szuro maintainer permissions, nor giving me permissions to fully manage this project.

I don't have time to work on such changes, this might explain a bit why I now lean towards the "do one thing, do it well" philosophy.

Freezepop commented 1 year ago

Hey @szuro, Yes, it has been a while.

My understanding is that people are looking for a zabbix sender, and only find the project at https://github.com/adubkov/py-zabbix. The adubkov/py-zabbix project haven't seen any release/changes merged since 2020, and one might assume it is dead.

Until we find a magical solution, I think we should add a link to the zappix project on Gitlab for people looking for a sender.

I do agree that we might want to follow the Unix philosophy, and make sure both projects (https://gitlab.com/szuro/zappix and https://github.com/lukecyca/pyzabbix) play nice with each other.

In any case, if we were to merge more code in this project, I would need the authors of this code to help me maintain it. But I haven't heard from @lukecyca about granting @szuro maintainer permissions, nor giving me permissions to fully manage this project.

I don't have time to work on such changes, this might explain a bit why I now lean towards the "do one thing, do it well" philosophy.

@jooola @szuro Guys, you want to make one good powerful library that would combine three projects.

This is a very good idea, but think about the users. People have been using py-zabbix for years now, a bunch of automations and integrations with other systems have already been developed based on this library.

Maybe someone will not like it, but I'll tell it like it is. People are switching from py-zabbix to pyzabbix because py-zabbix stopped supporting authorization.

They want to see the same functionality that the "Sender" module included, but it just doesn't exist.

If you want to make a more advanced library, then it's probably better to do it with a different name.

The associative array that people get when they hear pyzabbix/py-zabbix is related to already established functionality. Now people are experiencing dissonance in connection with this.

Therefore, I think that you just need to add "Sender" to pyzabbix, and develop new functionality as part of a new project.

This is just my opinion. Maybe I don't see the whole picture.

linceaerian commented 1 year ago

mmmh if @szuro https://gitlab.com/szuro/zappix works fine and is compatible with current "pyzabbix", we can just use it too (or add it in the doc for people looking for it), the only problem with multiple project owned by different maintainer is just that if it become "abandonned" it's hard to merge... while if it's one lib, the code is directly "built" to be compatible... (no incompat' between different lib usage for instance).

After it's just a question of "maintability" versus "lib size/complexity", afaik there's no right answer, just a personnal choice for this :)

lukecyca commented 1 year ago
  1. I have added @szuro as a collaborator to this project. Sorry for the delay.
  2. As for consolidating projects, I don't have a strong opinion and will mostly defer to the most active contributors of this project. My advice would be to keep the projects focussed. If the sender functionality is quite distinct and they're not going to share much code or concepts, then make them separate projects. Put links in the READMEs. Much of the success of this library can be attributed to keeping it simple and tight, and we should strive to keep it that way.
paalbra commented 1 year ago
  1. As for consolidating projects, I don't have a strong opinion and will mostly defer to the most active contributors of this project. My advice would be to keep the projects focussed. If the sender functionality is quite distinct and they're not going to share much code or concepts, then make them separate projects. Put links in the READMEs. Much of the success of this library can be attributed to keeping it simple and tight, and we should strive to keep it that way.

I've previously mentioned this, but again: I'd say that it's obvious that the HTTP-API and zabbix-sender/zabbix-protocol has nothing to do with each other. I'd put that in another project. I totally agree that this project should stay simple and just interface with the HTTP-API :+1:

szuro commented 1 year ago

So after reading all the comments, this i my current take:

pyzabbix and zappix

Leave zappix and pyzabbix as seperate packages, as their domains do not overlap. People migrating from py-zabbix might think that's the case, but they'd be wrong. It might be best to keep those separated and just add a note to the README. Alternatively, it could be just imported and used to implement a compatibility layer with py-zabbix like in pyzabbix/sender.py:

from zappix.sender import Sender

<some compatibility layer>

Hell, you could just copy the source from py-zabbix and call it a day.

But I think it would be for the best to just point people to zappix.

pyzabbix and zabbix-enums

zabbix-enums was meant to be a part of pyzabbix from the very beginning. When developing it, I decided against it - people were still using py-zabbix back then an wouldn't be able to benefit from said enums. As py-zabbix seems to be dead as @jooola pointed out, the reason no longer is valid.

zabbix-enums and pyzabbix would play nicely together and I think it would actually make sense to merge them.