Code cleanup and automated GitHub pylint workflow

[q1x]

I've added a workflow for automated GitHub pylint checks and worked out most of the pylint issues by doing some code cleanup. Other issues have been accepted and ignored for now :)

Would be great to merge and keep an eye on code consistency from now on, this will help when we start moving to automated container images.

[q1x]

Added Dockerfile and needed workflows to build GHCR images.

[TheNetworkGuy]

I'm going through https://github.com/TheNetworkGuy/netbox-zabbix-sync/pull/55/commits/de8143e89f76b46321c4a2ae54808068e5a924c5 and i see the following potentially issues:

Function setProxy() seems to have a different nesting. It seems logical to me that the error generated for not finding the proper proxy is called when the proxy is defined. Right now it will trigger when the keyword proxy is not found in the config context. I think that this should work:

    """ check if Zabbix Proxy has been defined in config context """
    if "zabbix" in self.nb.config_context:
        if "proxy" in self.nb.config_context["zabbix"]:
            proxy = self.nb.config_context["zabbix"]["proxy"]
            # Try matching proxy
            for px in proxy_list:
                if px["name"] == proxy:
                    self.zbxproxy = px["proxyid"]
                    logger.debug(f"Found proxy {proxy}"
                                    f" for {self.name}.")
                    return True
            return False
        e = f"{self.name}: Defined proxy {proxy} not found."
        logger.warning(e)
        return False
    return False

I see some import statements with raise Exception(e) from e. However e is overwritten by a string variable earlier on in the except clause. So this will result in TypeError: exception causes must derive from BaseException. Either the from e statement needs to be removed from the functions or the string variable name needs to be switched to something like "error" so that it won't interfear with the e variable which is earlier declared.

        except InterfaceConfigError as e:
            e = f"{self.name}: {e}"
            logger.warning(e)
            raise SyncInventoryError(e) from e

[TheNetworkGuy]

Could you elaborate a bit more in regards to https://github.com/TheNetworkGuy/netbox-zabbix-sync/pull/55/commits/0d7c581ee2d0bf3f0b2731110cd7107d249c8956? From what i can tell the pynetbox module should generate the exception (should something go wrong with creating a new journal entry) and not a new exception class.

[TheNetworkGuy]

Another note: something happend in the formatting of https://github.com/TheNetworkGuy/netbox-zabbix-sync/pull/55/commits/71f604a6f6945bd10d556b61dda21eea99c0393d which makes it very hard for me to see the difference in between commits. Is there any way how you can correct this? :)

[q1x]

Could you elaborate a bit more in regards to 0d7c581? From what i can tell the pynetbox module should generate the exception (should something go wrong with creating a new journal entry) and not a new exception class.

During testing I actually hit that exception and it failed for me (not sure what the error was exactly), creating a class for it seems to have fixed it.

[q1x]

Another note: something happend in the formatting of 71f604a which makes it very hard for me to see the difference in between commits. Is there any way how you can correct this? :)

I've reverted the file, please see 5b08d27a5e78cc1a80f301e30f4ed495021b04c6 as that shows the changes needed for Host Inventory as well as nested groupings.

[q1x]

I'm going through de8143e and i see the following potentially issues:

Function setProxy() seems to have a different nesting. It seems logical to me that the error generated for not finding the proper proxy is called when the proxy is defined. Right now it will trigger when the keyword proxy is not found in the config context. I think that this should work:

    """ check if Zabbix Proxy has been defined in config context """
    if "zabbix" in self.nb.config_context:
        if "proxy" in self.nb.config_context["zabbix"]:
            proxy = self.nb.config_context["zabbix"]["proxy"]
            # Try matching proxy
            for px in proxy_list:
                if px["name"] == proxy:
                    self.zbxproxy = px["proxyid"]
                    logger.debug(f"Found proxy {proxy}"
                                    f" for {self.name}.")
                    return True
            return False
        e = f"{self.name}: Defined proxy {proxy} not found."
        logger.warning(e)
        return False
    return False

I've just tested this, I don't think this is the case. I have config context setup with bare minimum:

{
    "zabbix": {
        "templates": []
    }
}

And it is not throwing proxy errors for me. I have full_proxy_sync = True in config.py.

[q1x]

And it is not throwing proxy errors for me. I have full_proxy_sync = True in config.py.

Found it and fixed in 091c9746c023e5ff55b83b1ed61c42eb738214d4

[q1x]

With regard to the raise SyncInventoryError(e) from e lines, those where recommended by pylint and I've just inserted those where it was complaining.

I will need to check what the idea behind it really is :)

[q1x]

With regard to the raise SyncInventoryError(e) from e lines, those where recommended by pylint and I've just inserted those where it was complaining.

I will need to check what the idea behind it really is :)

So, pylint was throwing these warning:

netbox_zabbix_sync.py:547:16: W0707: Consider explicitly re-raising using 'raise SyncExternalError(e) from e' (raise-missing-from)

See docs for more info.

I do seem to have added this statement on a few places where it wasn't needed, I've removed those in 634f4b77d5d7d6f60a427851546f92ec50e1386f. Pylint is still happy 🎉

[q1x]

Thanks a bunch!