Code review comment for ppa-dev-tools:add-ppa-credentials-command

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

Unfortunately this was kind of a "pull a loose thread from a sweater" problem, and ended up actually involving a more extensive amount of work than apparent. The credentials handling is pretty early on in the process (obviously) which made it tricky to tweak without breaking other things, but the good news is that the testsuite is extensive and thorough enough now that it was super helpful in spotting problems.

I've refactored the env var handling to be more general purpose, and layered in functionality to allow it to come from the specified file. I opted to name the option --credentials rather than --use-credentials to match the similar option --config.

I had initially hoped to split the review changes out separately to make it easier for you to check, but due to needing to add more fixes it was simpler to just merge all the rework into the main commits. Hopefully you can see what changed via range-diff, although since so much changed it may be worth just doing a fresh review from scratch.

« Back to merge proposal