andreaturli commented on this pull request.
it's a pretty big PR! I've looked at the implementation and it looks sensible
Few minor comments.
Can you please share the mock and live tests results?
> + private final AzureComputeApi api;
+ private final LoadingCache<String, ResourceGroup> resourceGroupMap;
+
+ @Inject
+ TemplateToAvailabilitySet(AzureComputeApi api, LoadingCache<String,
ResourceGroup> resourceGroupMap) {
+ this.api = api;
+ this.resourceGroupMap = resourceGroupMap;
+ }
+
+ @Nullable
+ @Override
+ public AvailabilitySet apply(final Template input) {
+ checkArgument(input.getOptions() instanceof AzureTemplateOptions, "An
AzureTemplateOptions object is required");
+ AzureTemplateOptions options =
input.getOptions().as(AzureTemplateOptions.class);
+
+ AvailabilitySet as = null;
[minor] can yo rename it `availabilitySet`?
> +import com.google.common.base.Function;
+import com.google.common.cache.LoadingCache;
+
+@Singleton
+public class TemplateToAvailabilitySet implements Function<Template,
AvailabilitySet> {
+
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final AzureComputeApi api;
+ private final LoadingCache<String, ResourceGroup> resourceGroupMap;
+
+ @Inject
+ TemplateToAvailabilitySet(AzureComputeApi api, LoadingCache<String,
ResourceGroup> resourceGroupMap) {
+ this.api = api;
I'd prefer not to inject `api` in functions as I've seen very funny problems as
the developer can forget that the function is actually using api calls. I've
seen it is only use for validation, maybe it can happen outside the function?
> @@ -217,6 +223,14 @@ private void configureSecurityGroupForOptions(String
> group, String resourceGroup
options.securityGroups(securityGroupId);
}
}
+
+ private void configureAvailabilitySetForTemplate(Template template) {
+ AvailabilitySet as = templateToAvailabilitySet.apply(template);
[minor] better name? :)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/356#pullrequestreview-19823053
