Bug#1055041: RFS: a2d/2.0.3-1 -- APRS to DAPNET portal
Hi Tobi,
On Wed, Nov 01, 2023 at 10:23:54AM +0100, Tobias Frost wrote:
Control: tags -1 moreinfo
Hi Yogeswaran,
I've took a look at your package:
Thank you for reviewing the package, I appreciate your feedback and
comments.
- You do not replace the debian changelog, you append the new
information. (hint: use "debchange" or "dch" for changelog manipulation)
I am using visual studio code to do all the modifications before using
gbp for packaging. That maybe the reason its replacing the changelog.
Unless its an issue, I would like to use visual studio code for now.
- there are undocumented changes, for example it seems that dependencies
has been changeed. that (and why it is) should be noted in
d/changelog.
I have fixed all the undocumented changes, and noted all of them in
debian changelog.
(You are also upstream)
Thank you for the feedback on upstream, it helped to improve the
upstream.
- It seems you have renamed a2dapp to a2d. This change is not good,
as it is too short / generic, and renaming is a breaking change
for your users. I'd appreaciate if you could stay with the old name.
I am not renaming the package from a2dapp to a2d. The original package
name was a2d from the begining. And a2dapp was not defined properly in
the previous version in setup.py (not according to Debian python). So in
upstream I have fixed it by moving the flask files to original a2d, and
also updated to toml instead of setup.py.
- Do you really need an hard dependency on nginx?
(I've checked myself: I think you don't. you'll use nginx as a proxy,
but people might want to setup e.g another http server for that or
even directly connect to the 9333 port (can they?); I think nginx
should be Recommends: not Depends: -- see policy about the definition
of Depends.)
Yes we need hard dependency on nginx. a2d is a flask app and it is
designed in a way that flask handles reverse proxy setup all the way to
generating signed CA SLL for the user. Without nginx the whole flask
will crash. It is by design to make it easy for ham radio users with
limited experience in linux systems. I have provided detailed
instructions for the users in the flask app on how changing ports and
working on SSL will impact their system.
- You README.md suggests to do rm -rf <various directories>
That is bad advice! Do not blindly say that they should nuke the nginx
configuration; (If, then the user should run apt purge nginx, but this
is also still dangerous a people might loose data); deluser is also
a bad advice, (system) users should not be deleted after they have
been created.
I have added clear warning to the user in the README.md file about
removing nginx. I have removed the part about deluser.
- postinst is wrong. Especially you do not remove those files on
"remove" -- only purge should delete user configuration, for example.
Thinking about it I am not sure if you are handling conffiles correctly,
as your postrm will nuke also files not handled by your packge, by
blindly removing them. This is an RC bug. Probably your application
will create files there? Maybe than /etc/ is the wrong location, maybe
app-created files should be reside in /var/lib/a2d? I'm not sure here
if/how this is handled correctly, maybe someone else will chime in.
I'm just sure that "rm -r /etc/a2d" could make some admins unhappy.
I believe you are talking about postrm, postinst dont remove anything.
(1) As you suggested, I have fixed the postrm so only during purge it
deletes user config and user's a2d database.
(2) I placed the user generated config files in /etc/, here the user use
flask to create their config files. So I believe it should be in /etc/
and not in /var/lib/a2d.
(I'm running out of time here, the review might not be complete)
Thank you again for your time!
Yogeswaran Umasankar.
Reply to: