Thank you for the thorough review, Ignasi – much appreciated. We’ll address the issues you flagged and provide an updated PR soon.
Thanks, Eduard From: Ignasi Barrera [mailto:[email protected]] Sent: Monday, February 16, 2015 5:28 AM To: jclouds/jclouds-labs Cc: Eduard Koller Subject: Re: [jclouds-labs] In progress implementation of JClouds for Azure (#132) Thankd @eduardkoller <https://github.com/eduardkoller> ! I've done an initial review focusing on structural things that must be addressed before this can be merged. Before we can merge it, all the comments must be addressed. I've added some general ones that I've mentioned only once to avoid repeating them: * All license headers must be the ASF headers. * Remove all printStackTrace references or replace them by proper log messages. * Remove all wildcard imports. * Remove all unused loggers, and properly qualify the remaining ones. Also move all debugging messages such as "entering here" to trace level. * Reduce the constructor visibility of all injection constructors to package-private. * Refactor the domain model classes to use Google Auto. Please, when committing fixes don't squash the commits. This way we'll be able to review just the last changes without having to go through the entire PR again. Once all global and particular comments have been addressed, and the next reviews are fine, I'll merge this. Once that happens, though, there will be several things that must be done: Tests: There is no single unit test in this code, and we really need them. The following tests must be added: * Unit tests for each transformation function. * Live tests for each *Api class method. * A live test that subclasses the BaseComputeServiceLiveTest. This is our contract, so it is extremely important. * A live test that subclasses the BaseTemplateBuilderLiveTest. Same as above, we need this test passing. Once all tests are in place, we can consider the provider as a "working provider". Regarding promotion to core, as discussed in the mailing list <http://markmail.org/message/et6pmzrygbnjvfuw> , there are more structural changes to be made, such as use the jclouds HTTP layer. It is not an issue now in labs, but I just want to leave the note now that there are two different implementations of the provider growing. — Reply to this email directly or view it on GitHub <https://github.com/jclouds/jclouds-labs/pull/132#issuecomment-74507936> . <https://github.com/notifications/beacon/ABIlTd1XORBvzsBXo_SZYVL_Jr-3ldfcks5nsef4gaJpZM4Dd6ic.gif> --- Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/132#issuecomment-74561510
