ulygit / asus_rt_ac68u

Configuration and script for Cloudflare DDNS on Asuswrt-Merlin
MIT License
58 stars 24 forks source link

Add ability to set CF "proxied" state #12

Closed bengalih closed 4 years ago

ulygit commented 4 years ago

Let's simplify the config by just having the user specify 'true' or 'false' instead of the numerical values. I suspect CF will provide an acceptable error response if the user enters an invalid value, and we can eliminate the translation in the script at the same time. Of course, we should add a comment about what values are valid in the config file and docs.

bengalih commented 4 years ago

If you want to go that route, I'm ok. Personally I've always like to minimize issues with invalid data in config files for public consumption. Initially I was going to do true/false, but then realized that I would need to do case-insensitive checks to determine if it was entered in any combination of those. Then, would need to convert to lower-case for the API call.

In the end I decided to just do boolean as it is hard for the user to mess that up and even if they do, the end result is just to execute on the default...so its just a soft fail.

With your methodology, a minor incorrect in the config script would be a hard fail. It's true that the existing code follows that, but I figured I might as well make it more error prone as new code was being added.

I'm not sure how specific the error message back from CF would be if the user entered "True" instead of "true", so they would just have to make due with whatever the logs showed.

In any event, it's your sandbox - so just let me know if you would prefer going the "true"/"false" route and I'll change that and update the docs to specify those values must be exact.

ulygit commented 4 years ago

Fixed separately. Thanks for your suggestion!