Attention is currently required from: Hoernchen.
Patch set 5:Verified +1Code-Review +1
8 comments:
Patchset:
I have tried the patchset and it works with SSL and with --nossl + nginx. I have tried the generate_smdp_certs.py script. It generates certificates but they don't work. I guess this is expected as it is intended only for testing.
File contrib/generate_smdpp_certs.py:
To me this looks like an independent tool. Maybe putting it in a separate patch would make sense?
File osmo-smdpp.py:
Patch Set #5, Line 26: from pathlib import Path
You have moved a lot of those imports from another code location. Maybe it would make sense to first move the imports in a patch before this one and then do the modifications?
We now support SSL and we have means to point to different certificate sub directories. Maybe we should make those two values configurable as well?
Patch Set #5, Line 586: parser.add_argument("-p", "--port", help="TCP port to bind HTTP to", default=8000)
Maybe 8443, since we now support SSL?
Patch Set #5, Line 587: parser.add_argument("-c", "--certpath", help=f"cert subdir relative to {DATA_DIR}", default="certs")
I think --certpath is a bit irritating as one may thing that it is possible to provide an absolute path to a random location in the file system, which actually not the case. The option describes a sub directory in smdp-data. Maybe something like --certdir would be more suitable?
Patch Set #5, Line 588: parser.add_argument("-s", "--nossl", help="do NOT use ssl", action='store_true', default=False)
I wonder if this may break anything when osmo-smdpp suddenly supports SSL by default. In any case I think that a --nossl option should be preferred over an --ssl option as SSL is the default and it should be on by default. So users will have to adopt.
Patch Set #5, Line 591: common_cert_path = os.path.join(DATA_DIR, args.certpath)
(see above) Maybe having DATA_DIR as a commandline parameter would be a good idea.
To view, visit change 40464. To unsubscribe, or for help writing mail filters, visit settings.