Code review comment for ~orndorffgrant/ubuntu/+source/ubuntu-advantage-tools:upload-28-mantic

Revision history for this message
Grant Orndorff (orndorffgrant) wrote :

Thank you Robie!

Yes that is an excellent point about our overall usage of "env" and we should fix it. After taking a look at all of our uses of that argument, I've discovered that it is exclusively there to support setting DEBIAN_FRONTEND=noninteractive for some apt subprocesses. With that in mind, I've created an upstream PR that changes the name and the semantics of the argument to be more in line with what we're using it for. That includes changing it so that the arg overrides os.environ instead of the other way around. I've also tried to address your points about mutating caller args and mutable default args in the refactor.

Please take a look here when you have time: https://github.com/canonical/ubuntu-pro-client/pull/2623

I also went ahead and included more explanatory comments in our systemd unit files in that same PR - separate commit. Let me know if you think they make sense to include now or if not then we can workshop them and add them later.

« Back to merge proposal