nacx requested changes on this pull request.
Thanks, @trevorflanagan! Apologies for the delay in the review. We've been
quite busy with the 2.0.2 release.
>
- public abstract int sizeGb();
+ @Nullable
+ public abstract Integer sizeGb();
Can this field have decimals? (Just asking; I'm not familiar with the API).
> + @Nullable @PayloadParam("disk") List<Disk> disks, @Nullable
> CreateServerOptions options);
+
+ @Named("server:delete")
+ @POST
+ @Path("/deleteServer")
+ @Produces(MediaType.APPLICATION_JSON)
+ @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
+ @MapBinder(BindToJsonPayload.class)
+ void deleteServer(@PayloadParam("id") String id);
+
+ @Named("server:powerOff")
+ @POST
+ @Path("/powerOffServer")
+ @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
In state change operations I think it would be better to fail if the server
does not exist. Otherwise, users won't be able to differentiate a failed
execution than a succeeded one. The use case here is different than in get
operations or delete (where the purpose is to have the server gone). Let's
remove the 404 fallbacks from all state change operations.
> + * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.predicates;
+
+import com.google.common.base.Predicate;
+import org.jclouds.dimensiondata.cloudcontrol.domain.Server;
+import org.jclouds.dimensiondata.cloudcontrol.features.ServerApi;
+import org.jclouds.logging.Logger;
+
+import javax.annotation.Resource;
+import java.text.MessageFormat;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class ServerStatus implements Predicate<String> {
Does this duplicate the `ServerState` class?
> + protected Logger logger = Logger.NULL;
+ private final ServerApi api;
+
+ public VMToolsRunningStatus(ServerApi api) {
+ this.api = api;
+ }
+
+ @Override
+ public boolean apply(String serverId) {
+ checkNotNull(serverId, "serverId");
+ logger.trace("looking for state on Server %s", serverId);
+ final Server server = api.getServer(serverId);
+ if (server == null) {
+ throw new IllegalStateException(MessageFormat.format("Server {0} is
not found", serverId));
+ }
+ return
server.guest().vmTools().runningStatus().equals(VmTools.RunningStatus.RUNNING);
The `vmTools` field is nullable. Protect this against a NPE.
> throw new IllegalStateException(message);
}
}
+ public static void waitForVmToolsRunning(ServerApi api, String serverId,
long timeoutMillis, String message) {
+ boolean vmwareToolsRunning = retry(new VMToolsRunningStatus(api),
timeoutMillis).apply(serverId);
+ if (!vmwareToolsRunning) {
+ throw new IllegalStateException(message);
+ }
+ }
Instead of having a static class with these accessors, consider configuring all
these predicates as injectable predicates. this makes it easier to get them
injected in the jclouds classes (such as the compute service when implemented),
and also allows users to configure the timeout values, etc, via properties.
You can have a look at [the digitalocean2
provider](https://github.com/jclouds/jclouds/blob/master/providers/digitalocean2/src/main/java/org/jclouds/digitalocean2/compute/config/DigitalOcean2ComputeServiceContextModule.java#L100-L216)
for an example.
> + @Test
+ public void testDeployAndStartServer() {
+ Boolean started = Boolean.TRUE;
+ NetworkInfo networkInfo = NetworkInfo
+ .create(NETWORK_DOMAIN_ID, NIC.builder().vlanId(VLAN_ID).build(),
Lists.<NIC>newArrayList());
+ List<Disk> disks =
ImmutableList.of(Disk.builder().scsiId(0).speed("STANDARD").build());
+ serverId = api().deployServer(deployedServerName, IMAGE_ID, started,
networkInfo, "P$$ssWwrrdGoDd!", disks, null);
+ assertNotNull(serverId);
+ waitForServerStatus(api(), serverId, true, true, 30 * 60 * 1000,
"Error");
+ waitForServerState(api(), serverId, State.NORMAL, 30 * 60 * 1000,
"Error");
+ }
+
+ @Test(dependsOnMethods = "testDeployAndStartServer")
+ public void testReconfigureServer() {
+ api().reconfigureServer(serverId, 4, CpuSpeed.HIGHPERFORMANCE.name(), 1);
+ waitForServerState(api(), serverId, State.PENDING_CHANGE, 30 * 60 *
1000, "Error");
Is it possible that the reconfigure operation is fast enough that we don't
actually see this state? Does it really make sense to wait for intermediate
states?
> + @Test(dependsOnMethods = "testStartServer")
+ public void testShutdownServer() {
+ api().shutdownServer(serverId);
+ waitForServerStatus(api(), serverId, false, true, 30 * 60 * 1000,
"Error");
+ }
+
+ @Test(dependsOnMethods = "testShutdownServer")
+ public void testCloneServer() {
+ CloneServerOptions options =
CloneServerOptions.builder().clusterId("").description("")
+ .guestOsCustomization(false).build();
+ cloneImageId = api().cloneServer(serverId, "ServerApiLiveTest-" +
System.currentTimeMillis(), options);
+ assertNotNull(cloneImageId);
+ waitForServerState(api(), serverId, State.NORMAL, 30 * 60 * 1000,
"Error");
+ }
+
+ @AfterClass
Annotate with `alwaysRun = true` to let it run even if tests fail.
> + }
+ }
+
+ public void testGetServer() throws Exception {
+ server.enqueue(jsonResponse("/server.json"));
+ Server found = serverApi().getServer("12345");
+ assertSent(GET,
"/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/server/12345");
+ assertNotNull(found);
+ assertNotNull(found.guest().vmTools());
+ }
+
+ public void testGetServer_404() throws Exception {
+ server.enqueue(response404());
+ serverApi().getServer("12345");
+ assertSent(GET,
"/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/server/12345");
+ }
Duplicates the `testGetServerReturnsResourceNotFound` test?
> + }
+
+ public void testListServers() throws Exception {
+ server.enqueue(jsonResponse("/servers.json"));
+ List<Server> servers = serverApi().listServers().concat().toList();
+ Uris.UriBuilder uriBuilder =
Uris.uriBuilder("/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/server");
+ Set<String> zones =
ctx.utils().injector().getInstance(ZoneIdsSupplier.class).get();
+ for (String zone : zones) {
+ uriBuilder.addQuery("datacenterId", zone);
+ }
+ assertSent(GET, uriBuilder.toString());
+ assertEquals(servers.size(), 1);
+ for (Server s : servers) {
+ assertNotNull(s);
+ }
+ }
Add a test for the list method when the response is a 404.
--
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/400#pullrequestreview-50242855
