Code review comment for ~bryce/ubuntu/+source/ssl-cert:ssl-cert-sru-lp1853021-hirsute

Revision history for this message
Bryce Harrington (bryce) wrote :

PPA is updated at https://launchpad.net/~bryce/+archive/ubuntu/ssl-cert-sru-lp1853021-snakeoil now

> I like adding proper arg handling, but IMHO this is one of the cases were we will end up with a lot of pain doing it as Ubuntu delta.

Completely agreed, same thought occurred to me, and yes I do keep a TODO to forward to Debian once the MP is acceptable.

> P.S. might be also worth to mention salsa in d/control

Good idea, the existing links are obsolete. I've updated them.

> The parsing right now is very bash'y e.g.
> --expiration-days=5 OK
> --expiration-days 5 FAILS
> Unrecognized option --expiration-days

I know, and I'm not happy about this as I strongly prefer the latter getopts-style format personally. However, I thought adding getopts might be a bridge too far for this package. I figure this was a less invasive change, but would be more than happy to move to getopts if Debian is ok with it. If you feel strongly that getopts *should* be used I'll update to use that.

Regarding shellcheck, yes I noticed there are numerous issues as well. I opted, however, to only fix shellcheck issues involved with the exact code lines I changed, to keep the patch minimal. I noticed the whitespace was handled irregularly elsewhere in the script so didn't worry about that, but I'll go and make sure all my touched lines are consistently using spaces instead of tabs. Followup patches to address both shellcheck and whitespace may make sense to consider doing when upstreaming the patch to Debian.

« Back to merge proposal