leamas / ddupdate

Update DNS Data for Dynamic IP Addresses
MIT License
40 stars 28 forks source link

add namecheap plugin #77

Closed timball closed 1 year ago

timball commented 1 year ago

This PR changes two things

  1. Adds a namecheap provider
  2. changes ddupdate to use /usr/bin/env instead of hardcoded python3 path

--timball

leamas commented 1 year ago

Hi Tim,

Thanks for PR, great work! Some questions/remarks:

First: the namecheap plugin seems to depend on the xmltodict PyPi module. These kind of dependencies are tricky to handle in a plugin, we have no way to force a user to install this first. In a proper packaging it is possible, but then raises a need to package this plugin in a separate sub-package. In short: would it be possible to use for example xml.etree which is part of the python standard library instead?

Second: Why use /usr/bin/env? There are good arguments against using it like those outlined here. That is, using /usr/bin/env requires a really good motivation.

Thoughts?

timball commented 1 year ago

Howdy @leamas !

  1. fixed the patch to not use xmltodict and use xml.etree instead
  2. https://peps.python.org/pep-0394/ says to use !/usr/bin/env to help those that deploy into virtual enviroments.

--timball

leamas commented 1 year ago

The namecheap plugin now looks good (besides a too long comment line, can fix that). Thanks!

But I don't really follow you about /usr bin/env. What I find in in pep-0394 is For scripts that are only expected to be run in an activated virtual environment, shebang lines can be written as #!/usr/bin/env python, as this instructs the script to respect the active virtual environment.

To me, this doesn't look that convincing, running ddupdate in a python venv is actually a corner case. What am I missing?

timball commented 1 year ago

I run ddupdate in a virtualenv. '!/usr/bin/env' helps the tool be a bit more flexible. For example osx doesn't have a '/usr/bin/python3'.

Eventually i will deploy ddupdate in a container and it will be via a virtualenv.

This isn't a hill to die on but those are some of the reasons having more flexibility would be a good thing.

leamas commented 1 year ago

Still on the /usr/bin/env issue... The reasons I quoted above was basically about that flexibility being a Bad Thing for most users. It is bad enough for distros to avoid it, they tend to prefer patching /usr/bin/env shebangs to get a more stable i. e., less flexible system

Note that ddupdate by definition is Linux-centric, so MacOS is nothing to consider. All to me known distros have a usable /usr/bin/python3.

As I see it, it does not really make sense to cause problems for the majority of users for your use case. If you want to deploy ddupdate in a a container, patching the shebang is probably a minor issue. That is., either we set the shebang to /usr/bin/env and forces all distros to patch it in the packaging, or leave it as is and let you and others with use cases like yours to patch it.

In the end, I think leaving the shebang as-is is the correct decision. Agreed?

OTOH, I'm interested in your efforts to deploy it in a container. Do you have a link?

timball commented 1 year ago

cool cool cool.

I committed a shebang revert so that you can merge the namecheap.py which is the primary motivation for my PR.

The container is pretty simple, just a clean k8 or docker with a virtualenv deployed into it. This allows us to deploy a minimal container to limit size; simplify deployments; have minimal SBOMs for deployment assurance.

--timball

timball commented 1 year ago

added edge case of co.uk second-level domain. Currently it is the only second-level domain that namecheap supports https://www.namecheap.com/domains/full-tld-list/

leamas commented 1 year ago

Thanks! Merge & close