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

providers: load as a single DB file. #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

systemcrash
Copy link
Contributor

( you may or may not want this, I simply propose it )

Gathering all providers into a single file has benefits for flash filesystems on openwrt. Many smaller files consume filesystem idents and may cause writes of more individual erase sectors than necessary when installed via the package manager. This likely won't happen if this app is baked into a sysupgrade, but that is typically an exception to the normal installation route.

Reading a single file is anyway faster, and compression across a larger file improves in general vs many smaller files (depending on how the rootfs is built), although the rootfs is compressed as a single stream anyway, so this is likely moot.

Removed a few redundant variables (name)

Fixed also Cloudlfare -> Cloudflare (in Tiarap)

Providers DB now has a single property to track version, which you can keep or discard.

Gathering all providers into a single file has benefits for flash
filesystems on openwrt. Many smaller files consume filesystem idents
and may cause writes of more individual erase sectors than necessary
when installed via the package manager. This likely won't happen if this
app is baked into a sysupgrade, but that is typically an exception to
the normal installation route.

Reading a single file is anyway faster, and compression across a larger
file improves in general vs many smaller files (depending on how the
rootfs is built), although the rootfs is compressed as a single stream
anyway, so this is likely moot.

Removed a few redundant variables (name)

Fixed also Cloudlfare -> Cloudflare (in Tiarap)
@stangri
Copy link
Owner

stangri commented Nov 30, 2024

The reasons for individual files are:

  • ease of contributions
  • only one file is changed with the update, so the rest are unlikely to get broken

Having said that, I'm thinking maybe I can prepare the combined file as a part of build/PR script but then the code would have to be updated to:

  1. Work with the individual files if providersdb.json is not found
  2. Be able to download the providersdb.json from openwrt luci repo on button click from WebUI

PS. Could you please elaborate on the fix for tiarap so that I could integrate it into the tiarap json?

@systemcrash
Copy link
Contributor Author

The reasons for individual files are:

* ease of contributions

I considered this high on my list.

Having said that, I'm thinking maybe I can prepare the combined file as a part of build/PR script but then the code would have to be updated to:

1. Work with the individual files if providersdb.json is not found

A fall-back. Well, if there's a problem with a single one of the provider files anyway, they all get read in to build a unified JSON object anyway (in the luci sh script), so we're back to square one, I think.

Was there some place in the build process you had in mind? I've not looked into that yet.

This was part of the idea behind handling zero length arrays. If no data comes in, it works fine now with #2. If broken data comes in, the whole interface loading breaks down with cascading errors. ( Irrespective of which file it's in ).

2. Be able to download the providersdb.json from openwrt luci repo on button click from WebUI

Easy enough. Existing logic should suffice. If you have something specific in mind I could probably cook it up.

PS. Could you please elaborate on the fix for tiarap so that I could integrate it into the tiarap json?

Look in the file for what should be "cloudflare" but isn't. :)

@stangri
Copy link
Owner

stangri commented Dec 1, 2024

Was there some place in the build process you had in mind? I've not looked into that yet.

Not sure how easy it would be to integrate into the Makefile itself, but I if it can be scripted as a few shell commands (and I'm sure it can, it's pretty much header + validate/add individual json files + footer) I can set things up so that the providersdb.json is generated when the packages are produced as a part of the PR process for the OpenWrt repo. I'd prefer the combined file be called providersdb.json (plural: providers, not singular provider).

As far as the header for the providersdb.json is concerned, thanks for including the version string in it, but I was also thinking it may have to include the compatibility integer there. I already use something like that to compare the version of the principal/luci apps in some of my packages to give warnings to users if a component is outdated (for localization needs, the warnings/errors from the principal app are not stored as a complete error text, but rather as a text ID, so if I add a new error/warning to the principal app, the luci app needs to be updated to have a localizable text for a new error/warning textID), so for https-dns-proxy there would need to be a compatibility check at least between luci app and the providersdb.json, so that if there are any changes to the individual or combined providers json files in the future, the older versions of the luci app would not attempt to load them). Easily implemented by declaring a variable in the luci RPCD script and printing it into the header of the providersdb.json.

PS. Finally saw the spelling issue with cloudflare in tiarap. ;)

@systemcrash
Copy link
Contributor Author

Pretty simple. Ci just needs this:
jq -s '.' file1.json file2.json > merged.json

Or just *.json > merged

You can add as many header strings as you like, so long as they remain outside of the provider array.

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