nacx requested changes on this pull request.
I've added some more comments. Regarding the live test failure, could you share
the complete stacktrace?
> @@ -67,7 +78,8 @@ protected Builder() {
.apiMetadata(new OneAndOneApiMetadata())
.homepage(URI.create("https://cloudpanel-api.1and1.com";))
.console(URI.create("https://account.1and1.com";))
- .endpoint("https://cloudpanel-api.1and1.com/v1";)
+ .iso3166Codes("de", "us", "es", "gb")
This should contain the iso codes, not the region id.
> +
+ // provision server
+ final Server server;
+ Double cores = ComputeServiceUtils.getCores(hardware);
+
+ try {
+ List<? extends Processor> processors = hardware.getProcessors();
+ org.apache.jclouds.oneandone.rest.domain.Hardware.CreateHardware
hardwareRequest
+ =
org.apache.jclouds.oneandone.rest.domain.Hardware.CreateHardware.create(processors.size(),
cores, hardware.getRam(), hdds);
+ final Server.CreateServer serverRequest =
Server.CreateServer.builder()
+ .name(name)
+ .hardware(hardwareRequest)
+ .rsaKey(options.getPublicKey())
+ .applianceId(image.getId())
+ .dataCenterId(dataCenterId)
+ .powerOn(Boolean.TRUE).build();
Set the password if the credentials are password-based?
> +
+ //if no private key was set use the set/generated password.
+ if (privateKey == null) {
+ serverCredentials = LoginCredentials.builder()
+ .user(loginUser)
+ .password(password)
+ .build();
+ } else {
+ serverCredentials = LoginCredentials.builder()
+ .user(loginUser)
+ .privateKey(privateKey)
+ .build();
+
+ }
+
+ return new NodeAndInitialCredentials<Server>(server, server.id(),
serverCredentials);
In this case there is no need to set the credentials manually here. jclouds
will populate the `defaultCredentials` for the image, or the ones provided in
the template options automatically. Just pass `null`.
Regarding the default credentials for the images, do they come with some
default user and a default password? Or are cloud-init images or similar that
create users and passwords on-the-fly?
> + }
+
+ @Override
+ public void rebootNode(String id) {
+ waitServerUntilAvailable.apply(getNode(id));
+ api.serverApi().updateStatus(id,
Server.UpdateStatus.create(Types.ServerAction.REBOOT,
Types.ServerActionMethod.HARDWARE));
+ }
+
+ @Override
+ public void resumeNode(String id) {
+ api.serverApi().updateStatus(id,
Server.UpdateStatus.create(Types.ServerAction.POWER_ON,
Types.ServerActionMethod.HARDWARE));
+ }
+
+ @Override
+ public void suspendNode(String id) {
+ waitServerUntilRunning.apply(getNode(id));
What if we call this on an already suspended node? Will it block until it times
out?
Perhaps it is a better idea to validate the state as a precondition and fail,
or not validate it at all and let the it fail in the server side.
> +
+ public ServerAvailablePredicate(OneAndOneApi api) {
+ this.api = checkNotNull(api, "api must not be null");
+ }
+
+ @Override
+ public boolean apply(Server server) {
+ checkNotNull(server, "Server");
+ server = api.serverApi().get(server.id());
+ if ((server.status().state() != Types.ServerState.POWERED_OFF
+ && server.status().state() != Types.ServerState.POWERED_ON)
+ || server.status().percent() != 0) {
+ return false;
+ } else {
+ return true;
+ }
Try to avoid conditionals like `if (condition) return true else return false`.
Just `return condition`.
> +
+ checkNotNull(serverRef, "serverRef");
+ //give time for the operation to actually start
+ Uninterruptibles.sleepUninterruptibly(10, TimeUnit.SECONDS);
+ Server server = api.serverApi().get(serverRef.id());
+
+ if (server == null) {
+ return false;
+ }
+ return server.status().toString().equals(expectedState.toString());
+ }
+
+ }
+
+ @Singleton
+ public static class ComputeConstants {
Get rid of this class and use the existing
[PollPeriod](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/reference/ComputeServiceConstants.java#L73-L82)
and
[Timeouts](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/reference/ComputeServiceConstants.java#L94-L105).
> + }).to(OneandoneComputeServiceAdapter.class);
+
+
bind(TemplateBuilderImpl.class).to(ArbitraryCpuRamTemplateBuilderImpl.class);
+
+ bind(new TypeLiteral<Function<Server, NodeMetadata>>() {
+ }).to(ServerToNodeMetadata.class);
+
+ bind(new
TypeLiteral<Function<org.apache.jclouds.oneandone.rest.domain.Image, Image>>() {
+ }).to(ImageToImage.class);
+
+ bind(new TypeLiteral<Function<Hdd, Volume>>() {
+ }).to(HddToVolume.class);
+
+ bind(new TypeLiteral<Function<HardwareFlavour, Hardware>>() {
+ }).to(HardwareFlavourToHardware.class);
+ }
If the public key configured by the user can be injected using the API (as it
seems), then add [this
binding](https://github.com/jclouds/jclouds/blob/master/providers/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/config/GoogleComputeEngineServiceContextModule.java#L121),
so jclouds does not try to upload it as part of a bootstrap ssh script.
> + HardwareFlavour flavour =
> api.serverApi().getHardwareFlavour(server.hardware().fixedInstanceSizeId());
+ hardware = fnHardware.apply(flavour);
+
+ } else {
+ //customer hardware
+ double size = 0d;
+ List<Volume> volumes = Lists.newArrayList();
+ List<Hdd> storages = server.hardware().hdds();
+ if (storages != null) {
+ for (Hdd storage : storages) {
+ size += storage.size();
+ volumes.add(fnVolume.apply(storage));
+ }
+ }
+ // Build hardware
+ String id = String.format("cpu=%f,ram=%d,disk=%f",
server.hardware().coresPerProcessor(), (int) server.hardware().ram(), size);
Generate the ID with
[this](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/util/AutomaticHardwareIdSpec.java#L60).
> @@ -19,6 +19,7 @@
public final class OneAndOneProperties {
public static final String POLL_PREDICATE_SERVER =
"jclouds.oneandone.rest.predicate.server";
+ public static final String POLL_PREDICATE_SERVER_RUNNING =
"jclouds.oneandone.rest.predicate.serveron ";
Remove trailing spaces
> +
+ public String getDescription() {
+ return description;
+ }
+
+ public static Location fromValue(String v) {
+ return Location.fromId(v);
+ }
+
+ public static Location fromId(String id) {
+ for (Location location : values()) {
+ if (location.id.equals(id)) {
+ return location;
+ }
+ }
+ return US;
Better throw an `IllegalArgumentException` instead.
--
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/338#pullrequestreview-16452526
