NebraLtd / hm-pktfwd

Helium Miner Packet Forwarder
https://nebra.io/hnt
MIT License
12 stars 25 forks source link

fix: tidy up Dockerfile and repo and add security check #22

Closed shawaj closed 3 years ago

shawaj commented 3 years ago

Relates-to: #20 Relates-to: #21 Signed-off-by: Aaron Shaw shawaj@gmail.com

shawaj commented 3 years ago

cc @marvinmarnold @vpetersson @robputt if you could take a look and see if this all makes sense 👍

i am sure there is more that can be improved, but hopefully this is a useful initial stab

vpetersson commented 3 years ago

@shawaj Don't leave commented code behind in the files. If we need to revert it later, we can pull the history from git.

shawaj commented 3 years ago

@vpetersson where did you find the commented code? Think I removed the last of it in the latest commit?

vpetersson commented 3 years ago

@vpetersson where did you find the commented code? Think I removed the last of it in the latest commit?

My bad. Either i reviewed a previous version or i misread.

vpetersson commented 3 years ago

@robputt / @marvinmarnold could either of you review this please?

shawaj commented 3 years ago

@vpetersson have squashed the commits to make it easier :-)

shawaj commented 3 years ago

@marvinmarnold @robputt the only thing I am not sure of now is how to fix the python security audit issues.

Have taken them down from "high" severity to "low" however still throwing up errors and out of my depth now.

I guess the answer is maybe to directly implement them in python rather than using subprocess, but not sure if that is possible?

EDIT: looks like it is not possible to pass this test due to an error in its implementation so perhaps we just need to add # nosec to these lines?

Also I think the B607 error is potentially a bug in bandit. Have filed a bug report at https://github.com/PyCQA/bandit/issues/726

marvinmarnold commented 3 years ago

EDIT: looks like it is not possible to pass this test due to an error in its implementation so perhaps we just need to add # nosec to these lines?

Agreed that #nosec is the best fix until this is fixed upstream.

shawaj commented 3 years ago

Agreed that #nosec is the best fix until this is fixed upstream.

@marvinmarnold ok added that now too.

Have re-requested review as think that is all sorted now and all checks / builds seem to be passing