Unit test decomposition looks good; thank you for that. I'd avoid wrapper functions in the future like install_mocks just so we can see mocked return_values are seen local to the unit test instead of having to look up at another method defintion to find out what it really is set to. I get that in this case it gives and easy eye in the 3 unit tests toward reading the True,False flags for whether the mock is 'active'.
Per the content of the merge proposal, the altered readurl in cloudinit.url_helper with the explicit session context manager doesn't adversely affect us on existing deployments +1 there. I also agree that any potential maintenance of the Scaleway datasource importing request.packages.urllib3 on other distributions will probably be minimal/infrequent, and if there is adverse impact, it will be limited to Scaleway cloud-init users.
Approved with Scott's comments for context and s/priviledged/privileged/.
Unit test decomposition looks good; thank you for that. I'd avoid wrapper functions in the future like install_mocks just so we can see mocked return_values are seen local to the unit test instead of having to look up at another method defintion to find out what it really is set to. I get that in this case it gives and easy eye in the 3 unit tests toward reading the True,False flags for whether the mock is 'active'.
Per the content of the merge proposal, the altered readurl in cloudinit. url_helper with the explicit session context manager doesn't adversely affect us on existing deployments +1 there. I also agree that any potential maintenance of the Scaleway datasource importing request. packages. urllib3 on other distributions will probably be minimal/infrequent, and if there is adverse impact, it will be limited to Scaleway cloud-init users.
Approved with Scott's comments for context and s/priviledged/ privileged/ .