Thankd @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

Reply via email to