nacx requested changes on this pull request.
Looks pretty good! Mostly comments about server authentication.
> + * defines the connection between the {@link org.jclouds.packet.PacketApi}
> implementation and
+ * the jclouds {@link org.jclouds.compute.ComputeService}
+ */
+@Singleton
+public class PacketComputeServiceAdapter implements
ComputeServiceAdapter<Device, Plan, OperatingSystem, Facility> {
+
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final PacketApi api;
+ private final Predicate<String> nodeRunningPredicate;
+ private final String projectId;
+
+ @Inject
+ public PacketComputeServiceAdapter(PacketApi api, @Provider final
Supplier<Credentials> creds, @Named(TIMEOUT_NODE_RUNNING) Predicate<String>
nodeRunningPredicate) {
Remove the public modifier and the redundant null checks.
> + private final PacketApi api;
+ private final Predicate<String> nodeRunningPredicate;
+ private final String projectId;
+
+ @Inject
+ public PacketComputeServiceAdapter(PacketApi api, @Provider final
Supplier<Credentials> creds, @Named(TIMEOUT_NODE_RUNNING) Predicate<String>
nodeRunningPredicate) {
+ this.api = checkNotNull(api, "api");
+ this.projectId = creds.get().identity;
+ this.nodeRunningPredicate = nodeRunningPredicate;
+ }
+
+ @Override
+ public NodeAndInitialCredentials<Device>
createNodeWithGroupEncodedIntoName(String group, String name, Template
template) {
+
+ PacketTemplateOptions templateOptions =
template.getOptions().as(PacketTemplateOptions.class);
+ checkNotNull(templateOptions.getLoginPrivateKey(), "login privateKey
must not be null");
Can't packet use a password based login?
> + String operatingSystem = template.getImage().getId();
+
+ Device device = api.deviceApi(projectId).create(
+ Device.CreateDevice.builder()
+ .hostname(name)
+ .plan(plan)
+ .billingCycle(billingCycle.value())
+ .facility(facility)
+ .features(features)
+ .operatingSystem(operatingSystem)
+ .locked(locked)
+ .userdata(userdata)
+ .tags(tags)
+ .build()
+ );
+ nodeRunningPredicate.apply(device.id());
Do we need to actually wait to have a complete Device object? If not, remove
this since the base class already performs the wait; we should not actively
wait here (otherwise we couldn't honor the options to not block, etc).
> + .features(features)
+ .operatingSystem(operatingSystem)
+ .locked(locked)
+ .userdata(userdata)
+ .tags(tags)
+ .build()
+ );
+ nodeRunningPredicate.apply(device.id());
+ device = api.deviceApi(projectId).get(device.id());
+
+ LoginCredentials defaultCredentials = LoginCredentials.builder()
+ .user("root")
+ .privateKey(templateOptions.getLoginPrivateKey())
+ .build();
+
+ return new NodeAndInitialCredentials<Device>(device, device.id(),
defaultCredentials);
You'd better pass `null` here to let jclouds determine the credentials based on
the options provided by the user and the default credentials configured per
context or per image.
> + PacketApi api, SshKeyPairGenerator keyGenerator) {
+ super(addNodeWithGroupStrategy, listNodesStrategy, namingConvention,
userExecutor,
+ customizeNodeAndAddToGoodMapOrPutExceptionIntoBadMapFactory);
+ this.api = api;
+ this.keyGenerator = keyGenerator;
+ }
+
+ @Override
+ public Map<?, ListenableFuture<Void>> execute(String group, int count,
Template template,
+ Set<NodeMetadata> goodNodes,
Map<NodeMetadata, Exception> badNodes,
+ Multimap<NodeMetadata,
CustomizationResponse> customizationResponses) {
+
+ PacketTemplateOptions options =
template.getOptions().as(PacketTemplateOptions.class);
+ Set<String> generatedSshKeyIds = Sets.newHashSet();
+
+ // If no key has been configured and the auto-create option is set,
then generate a key pair
There seems not to be such an auto-create option, so update the comment.
> + private final PacketApi api;
+ private final Predicate<String> nodeRunningPredicate;
+ private final String projectId;
+
+ @Inject
+ public PacketComputeServiceAdapter(PacketApi api, @Provider final
Supplier<Credentials> creds, @Named(TIMEOUT_NODE_RUNNING) Predicate<String>
nodeRunningPredicate) {
+ this.api = checkNotNull(api, "api");
+ this.projectId = creds.get().identity;
+ this.nodeRunningPredicate = nodeRunningPredicate;
+ }
+
+ @Override
+ public NodeAndInitialCredentials<Device>
createNodeWithGroupEncodedIntoName(String group, String name, Template
template) {
+
+ PacketTemplateOptions templateOptions =
template.getOptions().as(PacketTemplateOptions.class);
+ checkNotNull(templateOptions.getLoginPrivateKey(), "login privateKey
must not be null");
Also, we should be reading the "public" key and configuring the device with it.
The private key should remain in the client.
> +@Test(groups = "live", singleThreaded = true, testName =
> "PacketComputeServiceLiveTest")
+public class PacketComputeServiceLiveTest extends BaseComputeServiceLiveTest {
+
+ public PacketComputeServiceLiveTest() {
+ provider = "packet";
+ }
+
+ @Override
+ protected Module getSshModule() {
+ return new SshjSshClientModule();
+ }
+
+ @Override
+ protected Template buildTemplate(TemplateBuilder templateBuilder) {
+ return super.buildTemplate(templateBuilder);
+ }
No need to override this.
> + }
+ }
+
+ @Override
+ public void testOptionToNotBlock() throws Exception {
+ // Packet ComputeService implementation has to block until the node
+ // is provisioned, to be able to return it.
+ }
+
+ @Override
+ protected void checkUserMetadataContains(NodeMetadata node,
ImmutableMap<String, String> userMetadata) {
+ // The Packet API does not return the user data
+ }
+
+ @Override
+ public void testAScriptExecutionAfterBootWithBasicTemplate() throws
Exception {
Why do we need to override this?
--
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/354#pullrequestreview-19039151
