Refactor Pirania simplifying its code and fixing bugs #869
Conversation
Also fixes previous url (origin_url) that was being lost by mistake.
lua print function instroduced a expurius end of line and the response was broken.
|
After talking with Tania on how to deploy this new implementation on networks that already have pirania deployed I think it would be better to rename the shared-state database from vouchers to a new name for example pirania_vouchers. |
|
@spiccinini I have reviewed this PR finally. It was very easy to follow thanks to the documentation. The code is very clear too. |
In order to do this, add unique local and link local ipv6 to pirania's allow list addresses This fixes ubus requests in the portal
This brings pirania UI back to the portal
Pirania into shape more fixes
Simplifies the migration to the new pirania db as it allows running the new and the old version of pirania in the community in a mixed way.
|
I've pushed changes so that the shared-state db is now pirania-vouchers instead of pirania, so now both the new and the old version can run in the same community without crashing. |
Codecov Report
@@ Coverage Diff @@
## master #869 +/- ##
==========================================
+ Coverage 72.45% 73.92% +1.46%
==========================================
Files 41 41
Lines 3616 3559 -57
==========================================
+ Hits 2620 2631 +11
+ Misses 996 928 -68
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Impresive work!
have gone through it, did some comments.
By no means it has been a thorough review and no actual in device test was done.
I will unlock this so it can continue its development.
Would be good to add something on the README and on the Makefile to make sure that anyone that includes it knows that is dealing with alpha software.
packages/pirania/Readme.md
Outdated
| * the rest of the packets are rejeted (drop and send an error to the client) | ||
|
|
||
| ### HTTP flow |
There was a problem hiding this comment.
this is really good documentation. Maybe can eventually be moved to a /docs/ folder, as it sort of exceeds the responsabilities of a readme: https://www.freecodecamp.org/news/how-to-write-a-good-readme-file/ (of course it is very opinionated... maybe something to put our opinions into :) ).
There was a problem hiding this comment.
Yes, it should not be in a Readme but at the same time we are using the readme of the packages as documentation. Maybe when we implement an automatic generation of the full docs of libremesh it may be good idea to remove this details from the Readme but for the time being I think that nobody will see it (and I will forget about it).
packages/pirania/Readme.md
Outdated
| ``` | ||
|
|
||
| ### TODO |
There was a problem hiding this comment.
TODO: migrate the todo to issues in the repository :)
Thanks for this!
There was a problem hiding this comment.
Ok I will open an issue with an open Roadmap of pirania with some bullets and we may open issues for each relevant thing.
| print('true') | ||
| return true | ||
| else | ||
| return nil, 'false' |
There was a problem hiding this comment.
why is print('true') return true for true, and return nil, 'false' for false?
There was a problem hiding this comment.
This is because how the processing of the functions from the main functions works ...it somewhat hacky.
I am pushing a clearer implementation, thanks.
| for _, voucher in pairs(vouchera.vouchers) do | ||
| print(voucher.tostring()) | ||
| end | ||
| return true |
There was a problem hiding this comment.
show_active and show seem very similar... could they be merged?
| for _, voucher in pairs(vouchera.vouchers) do | ||
| if vouchera.is_active(voucher) then | ||
| print(voucher.mac) | ||
| end |
There was a problem hiding this comment.
same for show and show_active.. maybe they could all be merged?
| --! This will eventualy prune the voucher in all the dbs after PRUNE_OLDER_THAN_S seconds. | ||
| --! It is important to maintain the "removed" (deactivated) voucher in the shared db for some time | ||
| --! so that all nodes (even nodes that are offline when this is executed) have time to update locally | ||
| --! and eventualy prune the voucher. |
There was a problem hiding this comment.
it is also important to leave them there for some time in case the owners of the devices go and claim that they don't have internet anymore and say that the voucher is still valid. if we remove the voucher alltogether no trace will be kept and these kind of scams are easy to happen.
| local conn = ubus.connect() | ||
| if not conn then | ||
| error("Failed to connect to ubus") | ||
| local function unsafe_shell(command) |
There was a problem hiding this comment.
wasn't this function part of the utils module?
There was a problem hiding this comment.
Pirania does not have a direct dependency on the the lime-system package and I maintained it that way. But now shared-state depends on lime-system (well lime.utils only) we may also add this dependency and remove more code duplications. pirania-app is depending on lime-system so well it does not matter much now to maintain pirania package free of the lime dependency.
| local function add_vouchers(msg) | ||
| local vouchers = {} | ||
| for _, vouch_data in pairs (msg.vouchers_data) do | ||
| local v = {name=vouch_data.name, code=vouch_data.code, expiration_date=vouch_data.expiration_date} |
There was a problem hiding this comment.
looks like a table selective copy, like:
v = selective_copy(vouch_data, ["name", "code", "expiration_date"])
Just a thought... it has to be somewhere :)
| for _, voucher in pairs(vouchera.vouchers) do | ||
| if vouchera.is_active(voucher) then | ||
| table.insert(result.macs, voucher.mac) | ||
| end | ||
| end | ||
| printJson(result); |
There was a problem hiding this comment.
looks very similar to a function in the voucher binary, were similar information got printed.
There was a problem hiding this comment.
have you seen this @spiccinini ?
There was a problem hiding this comment.
Yes, in all cases it may be refactored into a generic apply_func_to_vouchers_if_condition(vouchers, func, condition), that is a map(func, filter(condition, vouchers)) (I don't like a lot this generic functions, I prefer to do them explicit in code). Maybe a more specific apply_to_active_vouchers(function) may be used (that I would prefer), but this won't serve all the cases mentioned. So for me it is not very clear that this refactor is a clear improvement (I mean maintainable without losing clarity and ease of reading).
| @@ -1,2 +1,2 @@ | |||
| #!/bin/sh | |||
| shared-state sync pirania | |||
| shared-state sync pirania-vouchers | |||
There was a problem hiding this comment.
The hooks folder should be documented within it, and outside of it somewhere, so people wanting to modify the behaviour can tell when that code is being executed or when code should be added here.
There was a problem hiding this comment.
have you seen this @spiccinini ?
There was a problem hiding this comment.
Hi Nico, yes. Indeed it should be Documented! This PR is not to fix every missing thing in Pirania, that is why I merged the branch even if not all improvement suggestions are done.
|
Thanks Nico for the review! |
This is a big PR with a full rewrite of the pirania lua code and improvements in other parts of the pirania codebase. The main drive of this rewrite is to simplify the code as there were many layers, some obsoleted, and tinkering through the code was difficult. The way to store into the shared-state storage is maintained but the local storage is moved from a csv into a per voucher json file.
This code is not compatible with the lime-app UI. We may write a layer to make it compatible but I think that we should drop the member/visitor for the renewable vouchers.
Please comment what you think, I don't expect a full code review as it is a big rewrite.
I believe that this rewrite fixes many issues with the old code but as we all know, new code comes with new bugs.