-
Notifications
You must be signed in to change notification settings - Fork 8
Update pbr: process_url #8
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
base: 1.1.7
Are you sure you want to change the base?
Conversation
Split process_url into two parts: set_dl that figures out which download tool to use process_url for downloading I did not know what to put in my configuration of PBR to test it on short notice. So this is UNTESTED. Also I traded in the sed regex for xargs which I interpreted as compatible. Download is redirected to stdout where its split into arguments by xargs. (no temp file needed anymore) Signed-off-by: bigsmile74 <118111679+bigsmile74@users.noreply.github.com>
-f FILE - FILE exists and is a regular file -x FILE - FILE exists and the user has execute (or search) access changed dl_scheme into dl_schemes
files/etc/init.d/pbr
Outdated
| set_dl(){ | ||
| [ -x "$wget" ] && { | ||
| dl_schemes='ftp http' | ||
| dl_command='$wget -q --no-check-certificate' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use double quotes for dl_command expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, otherwise it does not work 🤣
Changed single quotes, to variable friendly double quotes ;)
|
I like the introduction of dl_schemes per each download utility and just checking it against the schema used in the download. I think it would make sense to incorporate the code from dl_set() function back into process_download tho, and declare If you have curl installed you can test with a local file containing some domains. I can bump compat variables and improve/bring the error string into luci app after merging PR. |
|
(Would have gotten back sooner, but I'm swamped a the moment) Personally I would move dl_set into _system_health_check and check everything beforehand. Aborting then would prevent the system to become semi-configured. |
Now that there is a |
UNTESTED:
Split process_url into two parts:
I did not know what to put in my configuration of PBR to test it on short notice. So this is untested. Maybe you have suggestions for a couple of triggers.
Also I traded in the sed regex for xargs which I interpreted as compatible, but needs to be tested. Download is redirected to stdout where its split into arguments by xargs. (no temp file needed anymore)
Please if you think of a good error text, I could not.