Code review comment for lp:~tyhicks/ecryptfs/lp1597154-packaging

Revision history for this message
Jason Gerard DeRose (jderose) wrote :

Tyler,

Reading through the code, everything looks solid.

I also (synthetically) tested your proposed postinst script using `sudo dash ecryptfs-utils.postinst configure` on an appropriately configured system with an NVMe drive on which I first deliberately unset bit 63, and it worked fine. Your postinst script likewise worked fine when running it again after bit 63 was already set. (Although note that I didn't yet test using a built package with this change, with all the real-word mechanics of the postinst script being called in exact the circumstance that it would be for normal users.)

There are a few small issue I found with https://www.shellcheck.net/ (within the code added by this merge) that should probably be addressed eventually, but nothing that I consider deal breaking.

Finally, thanks for your comment about keeping ecryptfs-utils.postinst and ecryptfs-setup-swap in-sync. A very smart call there, in my opinion.

As soon as there's a package in xenial-proposed, I'll put this change through its paces in a 100% real-world scenario.

-Jason

review: Approve

« Back to merge proposal