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

CLI-rework #48

Merged
merged 40 commits into from
Apr 28, 2024
Merged

CLI-rework #48

merged 40 commits into from
Apr 28, 2024

Conversation

jp-berg
Copy link

@jp-berg jp-berg commented Mar 31, 2024

The intention is to be able to set default values (via the config-file), that can be overridden (via environment-variables or via the CLI). I applied this principle to the options already present in the config-file (endpoint, apikey, secretapikey), but my goal is to apply this pattern to all options. Values for an option are gathered first from the CLI. If not present there, the environment-variables will be checked and lastly the program tries to read the values from the config-file.

With this style I could fill in the necessary values into the config-file once and then override them when needed. Collecting all values into one file would also make backing up and sharing those values (e.g. between multiple installations) easier.

I tried to preserve compatibility with the old interface as best as I was able, but some use-cases might have slipped through.

I decided to follow the XDG Base Directory Specification for placement of the config-file to make the file-placement-decision easier and to encourage usage of standards.

I thought it was best to remove all file-handling-responsibility from the porkbun_ddns-module, since I felt that it does not fit into the area of responsibility of that class.

I tested the code on Debian 11 with Python 3.9.

Please let me know if you have any questions.

PS: This is my first pull-request, all feedback is welcome.

@mietzen
Copy link
Owner

mietzen commented Apr 1, 2024

Thank you for your contribution! I'm currently AFK, I will have a look at it next weekend.

@mietzen
Copy link
Owner

mietzen commented Apr 25, 2024

Sorry, that this is taking so long I had a look over but I need to change some stuff. At the moment my freetime is very limited, I'll have a deeper look at it a soon as possible.

@mietzen
Copy link
Owner

mietzen commented Apr 26, 2024

I took over most of your changes, except parsing the config via -c this would break the compatibility with previous versions.

For now the call would stay like this:

$ porkbun-ddns - domain.com my_subdomain

We need to drop python versions older than 3.10 to use the latest version of xdg, but I'm fine with that.
If you don't got any objections I will merges this and create a new release.

Edit: The more I think about it, the more I realise that dropping older Python versions is a even bigger change... I'll reintegrate the -c option is just way cleaner.

@mietzen
Copy link
Owner

mietzen commented Apr 26, 2024

You can test the latest version with pip install git+https://github.com/jp-berg/porkbun-ddns.git@cli-rework

@mietzen mietzen merged commit 7360ba0 into mietzen:main Apr 28, 2024
12 checks passed
@jp-berg
Copy link
Author

jp-berg commented Apr 28, 2024

Looks good to me, I will test this more as soon as I can. Thank you!

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.

2 participants