Code review comment for ~jcastets/cloud-init:scaleway-datasource

Revision history for this message
Chad Smith (chad.smith) wrote :

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/.

review: Approve

« Back to merge proposal