Pirania lime app integration #672
Conversation
nicopace
left a comment
nicopace
left a comment
There was a problem hiding this comment.
haven't finished reviewing... hope it helps! (you can also ignore completely :) )
| @@ -0,0 +1,18 @@ | |||
| { | |||
| "provider": { | |||
There was a problem hiding this comment.
can you describe the structure of this file? it is not clear what provider, community and member are, and how this affects the usage.
would be good to have a general documentaiton
| http://www.apache.org/licenses/LICENSE-3.0 | ||
| ]]-- | ||
|
|
||
| require "ubus" |
There was a problem hiding this comment.
could this file be called pirania instead of pirania-app?
in that way it would be pirania ubus interface, and it would make sense for others to use it for their own interfaces.
There was a problem hiding this comment.
There are two different apis. pirania deals with all the logic of the voucher system, while pirania-app deals more specifically with stuff related to the front-end app, such as content. Should it be merged into pirania?
There was a problem hiding this comment.
I feel that the '-app' doesn't represent what it is actually doing.
Adding a piece of documentation to it would help figure out why they are grouped the way they are... and may be merging or renaming them. Thanks!
There was a problem hiding this comment.
Docs have been added. Lemme know what u think. This decision was made by @altergui and me when we first got pirania working, and I built the vanilla JS app. The distinction between pirania and pirania-app may not make much sense now anymore.
There was a problem hiding this comment.
it might not be needed anymore indeed
There was a problem hiding this comment.
thanks for the docs!
It might be good to describe the whole governance.json.
it seems it has some clear structure with specific values that are used in different parts... would be good to know how them affect the use of the app.
There was a problem hiding this comment.
thanks for the docs!
It might be good to describe the whole governance.json.
it seems it has some clear structure with specific values that are used in different parts... would be good to know how them affect the use of the app.
| local function read_governance(msg) | ||
| local result = {} | ||
| local path = '/etc/pirania/governance.json' | ||
| result = utils.readJsonFile(path) |
There was a problem hiding this comment.
you don't need to read it as json, you can just print it out, as it is already a json :)
There was a problem hiding this comment.
it needs to be parsed though.
There was a problem hiding this comment.
If it is already in the filesystem, it should have been parsed before... so not sure if it is needed
| end | ||
|
|
||
| local function enable(msg) | ||
| uci_cursor:set("pirania", "base_config", "enabled", "1") |
There was a problem hiding this comment.
is it needed to both set enabled=1 and run captive-portal start?
@spiccinini
There was a problem hiding this comment.
Setting enabled makes pirania start on startup. To make it run you need to captive-portal start.
There was a problem hiding this comment.
Maybe we can use the /etc/init.d/ scripts for the startup?
spiccinini
left a comment
spiccinini
left a comment
There was a problem hiding this comment.
Great improvements over the base code. Still many things remain that should be improved, I have some commits that address some. Anyhow I think it should be better to merge this into the other branch as is and build from there.
Integration with LiMe-App. Check reviews from pirania PR.