You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by pimenas <no...@github.com> on 2014/04/07 22:37:18 UTC
[jclouds] openstack console (#339)
I've extended NovaApi.java and NovaAsyncApi.java to support method getConsole() which returns the vnc access url.
You can merge this Pull Request by running:
git pull https://github.com/pimenas/jclouds master
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/339
-- Commit Summary --
* added getConsole() in ServerApi, which retrieves the vnc url for a server.
* fixed author name
-- File Changes --
A apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/domain/Console.java (127)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/features/ServerApi.java (8)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/features/ServerAsyncApi.java (16)
A apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/functions/internal/ParseServerConsole.java (57)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/features/ServerApiExpectTest.java (57)
A apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/parse/ParseServerConsoleTest.java (60)
A apis/openstack-nova/src/test/resources/server_console.json (6)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/339.patch
https://github.com/jclouds/jclouds/pull/339.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> +/**
> + * Provides asynchronous access to Consoles via the REST API.
> + * <p/>
> + *
> + * @see ConsoleApi
> + * @author Epi Vou
> + * @see ExtensionAsyncApi
> + * @see <a href= "http://docs.openstack.org/api/openstack-compute/2/content/Extensions-d1e1444.html" />
> + * @see <a href="http://nova.openstack.org/api_ext" />
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html#ext-os-consoles" />
> + */
> +@Beta
> +@Extension(of = ServiceType.COMPUTE, namespace = ExtensionNamespaces.CONSOLES)
> +@RequestFilters(AuthenticateRequest.class)
> +public interface ConsolesAsyncApi {
@everett-toews @zack-shoylev @jdaggett: Do we still need an async version for the extension APIs?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11597935
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1206](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1206/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39795063
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-java-7-pull-requests #1314 SUCCESS
One new [Checkstyle violation](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1314/org.apache.jclouds.api$openstack-nova/violations/) here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44612929
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Is this a real failure?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44089879
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1058](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1058/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41893225
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #751](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/751/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40291303
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Is there a simple way to provide a custom deserialization adapter for the underlying gson? I need this in order to change the console type field to an enum type field.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40304858
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Actually I have one more thing to do, rebasing, never done this before so I have to read a little.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47437937
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #824](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/824/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44108938
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #846](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/846/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44634689
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +import com.google.common.base.Optional;
> +
> +/**
> + * Tests behavior of {@code ConsolesApi}
> + *
> + * @author Epi Vou
> + */
> +@Test(groups = "live", testName = "ConsolesApiLiveTest")
> +public class ConsolesApiLiveTest extends BaseNovaApiLiveTest {
> +
> + @Test
> + public void testGetVNCConsole() {
> + for (String zoneId : api.getConfiguredZones()) {
> + Optional<? extends ConsolesApi> apiOption = api.getConsolesExtensionForZone(zoneId);
> + if (!apiOption.isPresent())
> + continue;
Maybe print something or add a log message here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13035084
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
I can maybe shed some light on this. GSON will call fromValue to obtain the content type. The String type can (from what I remember) be null, in which case we want the Enum object as constructed by GSON to be null.
On the other hand, please make sure to also have the UNRECOGNIZED value implemented as well! With this approach all your bases are covered.
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13108493
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
Hi @pimenas! Thanks again for the contribution. :)
Since this feature is an extension to Nova, we should not tie it to the core `ServerApi` by adding a `getConsole()` method. Currently, jclouds supports the extension APIs as [optional](https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/NovaAsyncApi.java#L112).
I believe that the course of action we should take here is to move these changes into a new `ConsolesApi`. Accessing the new API would be similar to how the [other](https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/NovaAsyncApi.java#L112) APIs are accessed in the `NovaApi`.
The latest version of the [Consoles API](https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/contrib/consoles.py) indicates that there are 3 different consoles that you can request: VNC, SPICE, and RDP :)
With some minor refactoring we should be able to make this work as an extension API.
cc @everett-toews @zack-shoylev
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40238522
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1250](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1250/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41895416
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> So String type null -> Type.UNRECOGNIZED?
No, String type null would make the enum null. However, an empty string or a string that does not match any of the values will result in Type.UNRECOGNIZED
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13191736
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1295](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1295/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44108978
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + * Server Console Connection structure.
> + *
> + * @author Epi Vou
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html" />
> + */
> +public class Console {
> + public enum Type {
> + NOVNC("novnc"),
> + XVPVNC("xvpvnc"),
> + SPICE_HTML5("spice-html5"),
> + RDP_HTML5("rdp-html5");
> +
> + private static Map<String, Type> types;
> +
> + static {
> + types = newHashMap();
Use ImmutableMap.Builder?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11597480
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> fromValue() is used by the jclouds deserializer for the enums
Ah, OK. Thanks for explaining. Let's leave as-is, then. You learn something new every day!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11608689
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1260](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1260/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-42590792
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.rest.binders.BindToJsonPayload;
> +
> +import com.google.common.collect.ImmutableMap;
> +import com.google.common.collect.ImmutableSortedMap;
> +
> +@Singleton
> +public class BindConsoleToJsonPayload extends BindToJsonPayload {
> +
> + @Inject
> + public BindConsoleToJsonPayload(Json jsonBinder) {
> + super(jsonBinder);
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Map<String, Object> postParams) {
> + String action = null;
Does this need to be initialized, i.e. does the compiler complain if not? Because it would seem that each case of the switch initializes it, or throws an exception?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13034999
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
See my comment above. We `throw` an IAE in `BindConsoleToJsonPayload`, which personally I think is a more appropriate place.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13062759
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + public void testGetConsoleWhenResponseIs2xx() throws Exception {
> + String serverId = "123";
> + String type = "novnc";
> +
> + HttpRequest getConsole = HttpRequest
> + .builder()
> + .method("POST")
> + .addHeader("Accept", "application/json")
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
[minor] indents
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11371872
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1410](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1410/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47440075
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + }
> + }
> +
> + private final URI url;
> + private final Type type;
> +
> + @ConstructorProperties({
> + "url", "type"
> + })
> + protected Console(URI url, Type type) {
> + this.url = checkNotNull(url, "url");
> + this.type = checkNotNull(type, "type");
> + }
> +
> + /**
> + * Return an http url which will give access to the server console.
I think if you already have the `@return` bit, we don't need an additional statement. If we want to keep it, use "returns" (2nd person) instead of "return"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11597702
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + */
> +public class ParseServerConsole implements Function<HttpResponse, Console> {
> +
> + private final ParseJson<Map<String, Map<String, String>>> parser;
> +
> + @Inject
> + public ParseServerConsole(ParseJson<Map<String, Map<String, String>>> parser) {
> + this.parser = parser;
> + }
> +
> + @Override
> + public Console apply(HttpResponse response) {
> + checkNotNull(response, "response");
> +
> + Map<String, Map<String, String>> responseObj =
> + parser.apply(response);
[minor] No need to wrap here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11371861
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1317](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1317/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44634784
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/vnc_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
Always good to add more developer-friendliness to the base test!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13028932
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +@Test(groups = "live", testName = "ConsolesApiLiveTest")
> +public class ConsolesApiLiveTest extends BaseNovaApiLiveTest {
> +
> + @Test
> + public void testGetVNCConsole() {
> + for (String zoneId : api.getConfiguredZones()) {
> + Optional<? extends ConsolesApi> apiOption = api.getConsolesExtensionForZone(zoneId);
> + if (!apiOption.isPresent())
> + continue;
> +
> + ConsolesApi api = apiOption.get();
> + ServerApi serverApi = this.api.getServerApiForZone(zoneId);
> + Server server = createServerInZone(zoneId);
> + Console console = api.getConsole(server.getId(), Console.Type.NOVNC);
> + assertNotNull(console);
> + assertNotNull(console.getType());
The test below already covers this - remove?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13035075
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
Looks good.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44109395
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
I did the rebase and also squashed the change to 1 commit, so it should be ready for merging!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47446294
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #843](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/843/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44569346
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
Super helpful...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44108085
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/vnc_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
I think we don't use this style any more, but use `mockWebServer` tests instead. @zack-shoylev should have some good recent examples.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11598052
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + * Server Console Connection structure.
> + *
> + * @author Epi Vou
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html" />
> + */
> +public class Console {
> + public enum Type {
> + NOVNC("novnc"),
> + XVPVNC("xvpvnc"),
> + SPICE_HTML5("spice-html5"),
> + RDP_HTML5("rdp-html5");
> +
> + private static final Map<String, Type> types;
> +
> + static {
> + ImmutableMap.Builder<String, Type> builder = new ImmutableMap.Builder<String, Type>();
Would it handle correctly the detection of RDP_HTML5 from value "rdp-html5", by matching the underscore with the dash?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13025521
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + * Server Console Connection structure.
> + *
> + * @author Epi Vou
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html" />
> + */
> +public class Console {
> + public enum Type {
> + NOVNC("novnc"),
> + XVPVNC("xvpvnc"),
> + SPICE_HTML5("spice-html5"),
> + RDP_HTML5("rdp-html5");
> +
> + private static Map<String, Type> types;
> +
> + static {
> + types = newHashMap();
like this?
```
private static final Map<String, Type> types;
static {
ImmutableMap.Builder<String, Type> builder = new ImmutableMap.Builder<String, Type>();
for (Type t : values()) {
builder.put(t.type(), t);
}
types = builder.build();
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11606290
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + *
> + * @author Epi Vou
> + */
> +@Singleton
> +public class BindConsoleToJsonPayload extends BindToJsonPayload {
> +
> + @Inject
> + public BindConsoleToJsonPayload(Json jsonBinder) {
> + super(jsonBinder);
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Map<String, Object> postParams) {
> + String action = null;
> +
> + Console.Type type = (Console.Type) postParams.get("type");
I think in this case the custom MapBinder might be the solution, unfortunately. Which is very sad!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13028650
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1207](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1207/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39820753
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #840](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/840/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44374623
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + @Test
> + public void testGetVNCConsole() {
> + for (String zoneId : api.getConfiguredZones()) {
> + Optional<? extends ConsolesApi> apiOption = api.getConsolesExtensionForZone(zoneId);
> + if (!apiOption.isPresent())
> + continue;
> +
> + ConsolesApi api = apiOption.get();
> + ServerApi serverApi = this.api.getServerApiForZone(zoneId);
> + Server server = createServerInZone(zoneId);
> + Console console = api.getConsole(server.getId(), Console.Type.NOVNC);
> + assertNotNull(console);
> + assertNotNull(console.getType());
> + assertTrue(Console.Type.NOVNC.equals(console.getType()));
> + assertNotNull(console.getUrl());
> + assertTrue(console.getUrl().toString().startsWith("http"));
Why can we expect this? Is this likely to be brittle? What if this where a different (non-empty) value...would that mean the server was broken? Or our parsing logic?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13035073
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1022](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1022/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40291906
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Ok, I'll get on to it. In the mean-time you can all have a look of the how the code looks now.
I decided to provide three different functions, in accordance with the openstack consoles api.
These are getVNCConsole(), getSPICEConsole() and getRDPConsole().
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40301733
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1023](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1023/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40317427
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
Can this type-string-to-enum map be precalculated somewhere? It should not change at runtime, and then this code just becomes `return typeStringsToType.get(type)`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13035030
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1057](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1057/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41889578
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1142](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1142/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44167591
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/vnc_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
I used the freshly created "access.json" instead of "keystoneAuthResponse.json", because it uses internal URL's (e.g. "URL/v2/da0d12be20394afb851716e10a49e4a7"), and this way I overcame the previous exception. Now I have an issue with assertExtensions() method, I believe we need a different one, since it expects a "GET /extensions" but the actual request made in the compute api is "GET /v2/<tenant id>/extensions" .
I'll push my changes, for you to review and maybe suggest new assertion methods to be added to BaseOpenStackMockTest
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r12180095
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
@pimenas Is there anything else that needs to be done with this PR?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-46984779
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> +@Extension(of = ServiceType.COMPUTE, namespace = ExtensionNamespaces.CONSOLES)
> +@RequestFilters(AuthenticateRequest.class)
> +public interface ConsolesAsyncApi {
> + /**
> + * @see ConsolesApi#getConsole
> + */
> + @Named("server:console")
> + @POST
> + @Path("/servers/{serverId}/action")
> + @SelectJson("console")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Produces(MediaType.APPLICATION_JSON)
> + @Fallback(MapHttp4xxCodesToExceptions.class)
> + @MapBinder(BindConsoleToJsonPayload.class)
> + ListenableFuture<? extends Console> getConsole(@PathParam("serverId") String serverId,
> + @PayloadParam("type") Console.Type type);
Instead of a MapBinder does something like @WrapWith work? Seeing as GSON should be able to serialize the Console.Type.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r12915683
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1296](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1296/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44118279
Re: [jclouds] openstack console (#339)
Posted by Everett Toews <no...@github.com>.
Good catch @jdaggett!
Jeremy is right. The Console API is an OpenStack extension and needs to be coded as such in jclouds. The particular call you're making is for the [os-consoles extension](http://api.openstack.org/api-ref-compute-v2-ext.html#ext-os-consoles).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40247575
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + *
> + * @author Epi Vou
> + */
> +@Singleton
> +public class BindConsoleToJsonPayload extends BindToJsonPayload {
> +
> + @Inject
> + public BindConsoleToJsonPayload(Json jsonBinder) {
> + super(jsonBinder);
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Map<String, Object> postParams) {
> + String action = null;
> +
> + Console.Type type = (Console.Type) postParams.get("type");
There might still be a way to simplify this further by convincing GSON to help out more! But I haven't tried it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r12869371
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
Can `type` ever be `null` here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13035012
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> The String type can (from what I remember) be null
So String type `null` -> Type.UNRECOGNIZED?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13111858
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + }
> + }
> +
> + private static class ConcreteBuilder extends Builder<ConcreteBuilder> {
> + @Override
> + protected ConcreteBuilder self() {
> + return this;
> + }
> + }
> +
> + private final URI url;
> + private final Type type;
> +
> + @ConstructorProperties({
> + "url", "type"
> + })
[minor] Formatting? I guess this could all go on one line?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11597656
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + public void testGetConsoleWhenResponseIs404NotFound() throws Exception {
> + String serverId = "d3aae414-9dc9-46d1-a51b-e9b7071a84b7";
> + String type = "novnc";
> +
> + HttpRequest getConsole = HttpRequest
> + .builder()
> + .method("POST")
> + .addHeader("Accept", "application/json")
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
[minor] indents
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11371875
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1248](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1248/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41890622
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +import org.jclouds.openstack.v2_0.ServiceType;
> +import org.jclouds.openstack.v2_0.services.Extension;
> +
> +import com.google.common.annotations.Beta;
> +
> +/**
> + * Provides synchronous access to Consoles.
> + * <p/>
> + *
> + * @see ConsoleAsyncApi
> + */
> +@Beta
> +@Extension(of = ServiceType.COMPUTE, namespace = ExtensionNamespaces.CONSOLES)
> +public interface ConsolesApi {
> + /**
> + * Get Console
Gets the console :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13038601
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #777](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/777/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41883733
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + Console.Type type = (Console.Type) postParams.get("type");
> +
> + switch (type) {
> + case NOVNC:
> + case XVPVNC:
> + action = "os-getVNCConsole";
> + break;
> + case SPICE_HTML5:
> + action = "os-getSPICEConsole";
> + break;
> + case RDP_HTML5:
> + action = "os-getRDPConsole";
> + break;
> + default:
> + throw new IllegalArgumentException("Invalid type: " + type);
> If at this point the type returned from openstack is not valid, then maybe we should throw some exception which
> could be IAE since from a little search I did, I haven't found something more appropriate.
That sounds better than somehow silently ignoring it. But in general I don't think we program defensively for "broken" server responses - if the server returns unexpected things, jclouds may indeed break. See comment for `fromValue` below.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13103748
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #940](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/940/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47440724
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/spice_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
> +
> + assertEquals(api.getConsolesExtensionForZone("az-1.region-a.geo-1")
> + .get().getConsole(serverId, Console.Type.SPICE_HTML5).toString(),
> + new ParseSPICEConsoleTest().expected().toString());
If we remove the toString() nothing would happen. We compare two identical instances of Console class, so if `Console.equals()` is implemented correctly it should return the same result.
Personally I would prefer removing toString(), because then the test would also fail if `Console.equals()` method is not implemented correctly
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13062822
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +import org.jclouds.rest.binders.BindToJsonPayload;
> +
> +import com.google.common.collect.ImmutableMap;
> +import com.google.common.collect.ImmutableSortedMap;
> +
> +@Singleton
> +public class BindConsoleToJsonPayload extends BindToJsonPayload {
> +
> + @Inject
> + public BindConsoleToJsonPayload(Json jsonBinder) {
> + super(jsonBinder);
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Map<String, Object> postParams) {
> + String action = null;
You're correct, I removed the initialization.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13038192
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1247](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1247/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41883703
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> suggested to switch to make it like in the example
Let's stick with @zack-shoylev's suggestion, then ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13058907
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + return "/spice_console.json";
> + }
> +
> + @Override
> + @SelectJson("console")
> + @Consumes(MediaType.APPLICATION_JSON)
> + public Console expected() {
> + try {
> + return Console
> + .builder()
> + .url(new URI("http://example.com:6080/spice_auto.html?token=f9906a48-b71e"
> + + "-4f18-baca-c987da3ebdb3&title=dafa(75ecef58-3b8e-4659-ab3b-5501454188e9)"))
> + .type(Console.Type.SPICE_HTML5)
> + .build();
> + } catch (Exception e) {
> + throw new RuntimeException(e);
[minor] see above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11598117
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1000](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1000/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39786549
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + Console.Type type = (Console.Type) postParams.get("type");
> +
> + switch (type) {
> + case NOVNC:
> + case XVPVNC:
> + action = "os-getVNCConsole";
> + break;
> + case SPICE_HTML5:
> + action = "os-getSPICEConsole";
> + break;
> + case RDP_HTML5:
> + action = "os-getRDPConsole";
> + break;
> + default:
> + throw new IllegalArgumentException("Invalid type: " + type);
How can this happen? I'm assuming this is defensive in case someone modifies the enum without changing this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13034978
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + * Server Console Connection structure.
> + *
> + * @author Epi Vou
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html" />
> + */
> +public class Console {
> + public enum Type {
> + NOVNC("novnc"),
> + XVPVNC("xvpvnc"),
> + SPICE_HTML5("spice-html5"),
> + RDP_HTML5("rdp-html5");
> +
> + private static final Map<String, Type> types;
> +
> + static {
> + ImmutableMap.Builder<String, Type> builder = new ImmutableMap.Builder<String, Type>();
I'll make it like the example.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13029330
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #736](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/736/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39794578
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
This seems like an unrelated failure.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44108004
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1168](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1168/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44636799
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1412](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1412/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47440947
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/spice_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
> +
> + assertEquals(api.getConsolesExtensionForZone("az-1.region-a.geo-1")
> + .get().getConsole(serverId, Console.Type.SPICE_HTML5).toString(),
> + new ParseSPICEConsoleTest().expected().toString());
I am not sure this will matter once this is switched over to the mock style tests, correct?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13193970
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + */
> +@Test(groups = "live", testName = "ConsolesApiLiveTest")
> +public class ConsolesApiLiveTest extends BaseNovaApiLiveTest {
> +
> + @Test
> + public void testGetVNCConsole() {
> + for (String zoneId : api.getConfiguredZones()) {
> + Optional<? extends ConsolesApi> apiOption = api.getConsolesExtensionForZone(zoneId);
> + if (!apiOption.isPresent())
> + continue;
> +
> + ConsolesApi api = apiOption.get();
> + ServerApi serverApi = this.api.getServerApiForZone(zoneId);
> + Server server = createServerInZone(zoneId);
> + Console console = api.getConsole(server.getId(), Console.Type.NOVNC);
> + assertNotNull(console);
removing..
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13038825
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/spice_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
> +
> + assertEquals(api.getConsolesExtensionForZone("az-1.region-a.geo-1")
> + .get().getConsole(serverId, Console.Type.SPICE_HTML5).toString(),
> + new ParseSPICEConsoleTest().expected().toString());
Any reason why here (and elsewhere in this test) we're coupling the expect result of this to a different unit test? I'd rather have the expected result in _this_ class, so I can easily look up what it is?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13035057
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #823](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/823/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44108921
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + Console.Type type = (Console.Type) postParams.get("type");
> +
> + switch (type) {
> + case NOVNC:
> + case XVPVNC:
> + action = "os-getVNCConsole";
> + break;
> + case SPICE_HTML5:
> + action = "os-getSPICEConsole";
> + break;
> + case RDP_HTML5:
> + action = "os-getRDPConsole";
> + break;
> + default:
> + throw new IllegalArgumentException("Invalid type: " + type);
> if the someone pass a null value as the type argument of getConsole() function
See comment below. Is `null` a valid input to `getConsole`? If not, we should be throwing an error long before we get here, I'd say?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13058923
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + @Test
> + public void testGetVNCConsole() {
> + for (String zoneId : api.getConfiguredZones()) {
> + Optional<? extends ConsolesApi> apiOption = api.getConsolesExtensionForZone(zoneId);
> + if (!apiOption.isPresent())
> + continue;
> +
> + ConsolesApi api = apiOption.get();
> + ServerApi serverApi = this.api.getServerApiForZone(zoneId);
> + Server server = createServerInZone(zoneId);
> + Console console = api.getConsole(server.getId(), Console.Type.NOVNC);
> + assertNotNull(console);
> + assertNotNull(console.getType());
> + assertTrue(Console.Type.NOVNC.equals(console.getType()));
> + assertNotNull(console.getUrl());
> + assertTrue(console.getUrl().toString().startsWith("http"));
Well the "novnc" console is either http or https url, so if it's something different then something is wrong (either our parsing logic, or the server). On the other side, I don't think it's likely to be brittle, but it doesn't hurt to test it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13038799
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
@pimenas Just a quick comment about providing the following methods:
getVNCConsole(), getSPICEConsole() and getRDPConsole()
I would provide a single method "getConsole(ConsoleType type)" because that leaves the interface open to other types of consoles that might be added.
cc @demobox
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40396434
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + * Server Console Connection structure.
> + *
> + * @author Epi Vou
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html" />
> + */
> +public class Console {
> + public enum Type {
> + NOVNC("novnc"),
> + XVPVNC("xvpvnc"),
> + SPICE_HTML5("spice-html5"),
> + RDP_HTML5("rdp-html5");
> +
> + private static final Map<String, Type> types;
> +
> + static {
> + ImmutableMap.Builder<String, Type> builder = new ImmutableMap.Builder<String, Type>();
I am not completely certain the static map is needed. Thoughts? See my other comment.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r12869108
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Thanks, that will work!
Epimenidis Voutsakis
Electronics & Computer Engineer
pimenas@gmail.com
+30 695 720 6307 (Mobile, Greece)
On Sun, Apr 13, 2014 at 3:40 PM, Ignasi Barrera <no...@github.com>wrote:
> Jclouds already has a built-in enum deserializer. It will use the valueOfmethod, but will fallback to a
> fromValue one if the former does not work. If the values in the json
> document don't match the enum names, the easiest way is to provide a proper
> fromValue method in the enum. Does this work for you?
>
> --
> Reply to this email directly or view it on GitHub<https://github.com/jclouds/jclouds/pull/339#issuecomment-40306683>
> .
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40307662
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +
> + public void testGetConsoleWhenResponseIs2xx() throws Exception {
> + String serverId = "123";
> + String type = "novnc";
> +
> + HttpRequest getConsole = HttpRequest
> + .builder()
> + .method("POST")
> + .addHeader("Accept", "application/json")
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11378279
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #827](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/827/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44168726
Re: [jclouds] openstack console (#339)
Posted by Ignasi Barrera <no...@github.com>.
I think the cleanest way will be to create your own [MapBinder](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/rest/MapBinder.java): You'll have to create a class that implements that interface (usually also subclassing the [BindToJsonPayload](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/rest/binders/BindToJsonPayload.java) class), and then remove the `@Payload` annotation from the api method and add the following one (the `@Payload` annotations in the method parameters should remain there):
```java
@MapBinder(YourCustomBinder.class)
```
Implementing the binder is pretty straightforward. The method will receive a map already populated with the keys (defined in the `@Payload` annotations in the parameters) and the corresponding values; you just need to manually generate the payload string. You can take a look at [some existing binders](https://github.com/jclouds/jclouds/tree/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/binders) in the openstack-nova api to see some examples of how they work.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41508510
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
I used the example from the jclouds wiki https://wiki.apache.org/jclouds/Enums,
and I also think someone could pass a null value in ConsolesApi.getConsole().
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13038440
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/vnc_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
Hi @pimenas, I think the issue is with the line:
String zoneId = getFirst(novaApi.getConfiguredZones(), "RegionTwo");
in the keystoneAuthResponse.json, what are the region names for compute? I suspect that "RegionTwo" is not a valid region in that file.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r12158685
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> + if (type.equals(value.type)) {
> + return value;
> + }
> + }
> + return UNRECOGNIZED;
> + }
> + return null;
> + }
> So any suggestions on the logger, or should I remove it?
Ah...eek...yes, good point. It's either the `Logger.getAnonymousLogger()` or none. Do we know what `getAnonymousLogger()` does? Not great to have the logging not linked to a class, but I'd rather get this warning from an anonymous logger than to ignore it ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13332826
Re: [jclouds] openstack console (#339)
Posted by Everett Toews <no...@github.com>.
Yes to both URI and Enum. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40292073
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + * Server Console Connection structure.
> + *
> + * @author Epi Vou
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html" />
> + */
> +public class Console {
> + public enum Type {
> + NOVNC("novnc"),
> + XVPVNC("xvpvnc"),
> + SPICE_HTML5("spice-html5"),
> + RDP_HTML5("rdp-html5");
> +
> + private static Map<String, Type> types;
> +
> + static {
> + types = newHashMap();
> like this?
Yes, exactly ;-) Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11608645
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
I plan to add tests for all types, after all they are tested in a similar way. The reason I haven't done it yet, is that I want you guys to review the test code (especialy the mockwebserver style tests) and tell me if I should change/add/remove something.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44165494
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1061](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1061/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-42099388
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1165](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1165/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44566802
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +
> + Console.Type type = (Console.Type) postParams.get("type");
> +
> + switch (type) {
> + case NOVNC:
> + case XVPVNC:
> + action = "os-getVNCConsole";
> + break;
> + case SPICE_HTML5:
> + action = "os-getSPICEConsole";
> + break;
> + case RDP_HTML5:
> + action = "os-getRDPConsole";
> + break;
> + default:
> + throw new IllegalArgumentException("Invalid type: " + type);
It is not a valid input to `getConsole()`, and this is why I put the `throw` in this place. If you think the fromValue() in Console.Type enum is a better place to throw this exception I'll change this. In my opinion however the binder is a more appropriate place to validate the arguments and throw an exception, than the fromValue() in the enums.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13062737
Re: [jclouds] openstack console (#339)
Posted by Ignasi Barrera <no...@github.com>.
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/vnc_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
You can also have a look at these [extension mock tests](https://github.com/jclouds/jclouds/tree/master/apis/openstack-keystone/src/test/java/org/jclouds/openstack/keystone/v2_0/extensions), as they include an `assertExtensions` you'll need to use too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11622080
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + *
> + * @author Epi Vou
> + */
> +@Singleton
> +public class BindConsoleToJsonPayload extends BindToJsonPayload {
> +
> + @Inject
> + public BindConsoleToJsonPayload(Json jsonBinder) {
> + super(jsonBinder);
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Map<String, Object> postParams) {
> + String action = null;
> +
> + Console.Type type = (Console.Type) postParams.get("type");
I use the MapBinder because depending on the value of the Console type ("novnc", "xvpvnc", "spice-html5", "rdp-html5"), we need to wrap the json object with different values ("os-getVNCConsole" when type is "novnc" or "xvpvnc", "os-getVNCConsole" when type is "spice-html5", and "os-getVNCConsole" when type is "rdp-html5".
There are comments above, where @nacx recommends using a custom MapBinder, and I do not see a simpler way of accomplishing this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13025507
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> @@ -0,0 +1,6 @@
> +{
I just use an eclipse addon to reformat as needed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r12868900
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
@pimenas Sounds like a plan, no objections! The `getConsole()` method definitely leaves the options open for new types of consoles in the future.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40733191
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1001](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1001/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39794905
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
> +
> +/**
> + * Provides asynchronous access to Consoles via the REST API.
> + * <p/>
> + *
> + * @see ConsoleApi
> + * @author Epi Vou
> + * @see ExtensionAsyncApi
> + * @see <a href= "http://docs.openstack.org/api/openstack-compute/2/content/Extensions-d1e1444.html" />
> + * @see <a href="http://nova.openstack.org/api_ext" />
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html#ext-os-consoles" />
> + */
> +@Beta
> +@Extension(of = ServiceType.COMPUTE, namespace = ExtensionNamespaces.CONSOLES)
> +@RequestFilters(AuthenticateRequest.class)
> +public interface ConsolesAsyncApi {
@demobox Yes. I already unasynced most of Nova and the extensions. :) Still working through that, but for now, I would say keep the async interface. I will pull these changes in and unasync in another PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11598287
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +import java.beans.ConstructorProperties;
> +
> +import com.google.common.base.Objects;
> +import com.google.common.base.Objects.ToStringHelper;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * Connection information to connect to a server using VNC.
> + *
> + * @author Epi Vou
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html" />
> + */
> +public class Console {
> +
> + public static Builder<?> builder() {
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11378267
Re: [jclouds] openstack console (#339)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> + if (type.equals(value.type)) {
> + return value;
> + }
> + }
> + return UNRECOGNIZED;
> + }
> + return null;
> + }
FTR: [Logger.html#getAnonymousLogger](http://docs.oracle.com/javase/7/docs/api/java/util/logging/Logger.html#getAnonymousLogger())
How important are those warning logs? If we use the anonymous logger we'll be potentially using a logging framework different than the one the user configured when creating the context, and that may end up in unexpected messages appearing in the wrong place.
If we completely remove the logging here (the only remaining alternative), how bad would that be? If at some point one has to debug what's going on, could the wire logs be inspected to find the responses containing unexpected values?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13333769
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> and I also think someone could pass a null value in ConsolesApi.getConsole()
But we don't consider that a valid input value, or do we? We should be throwing an IAE or some other error if that happens, I guess?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13058913
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + */
> +public class ParseServerConsole implements Function<HttpResponse, Console> {
> +
> + private final ParseJson<Map<String, Map<String, String>>> parser;
> +
> + @Inject
> + public ParseServerConsole(ParseJson<Map<String, Map<String, String>>> parser) {
> + this.parser = parser;
> + }
> +
> + @Override
> + public Console apply(HttpResponse response) {
> + checkNotNull(response, "response");
> +
> + Map<String, Map<String, String>> responseObj =
> + parser.apply(response);
fixed, I'm just used to 80 characters per line..
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11378276
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #778](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/778/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41887726
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1294](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1294/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47440996
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Hi!
Sorry for the late response, but I couldn't find time... As @demobox pointed out we need
* different implementation of fromValue(). Already done that, haven't pushed the changes yet.
* use MockWebServer style tests instead of expect tests. I'll remove the expect tests and implement them using MWS style. I've already have implemented 2 MWS tests, but I think we need more.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47250596
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + return "/rdp_console.json";
> + }
> +
> + @Override
> + @SelectJson("console")
> + @Consumes(MediaType.APPLICATION_JSON)
> + public Console expected() {
> + try {
> + return Console
> + .builder()
> + .url(new URI("http://example.com:6083/?token=f9906a48-b71e-4f18-baca-"
> + + "c987da3ebdb3&title=dafa(75ecef58-3b8e-4659-ab3b-5501454188e9)"))
> + .type(Console.Type.RDP_HTML5)
> + .build();
> + } catch (Exception e) {
> + throw new RuntimeException(e);
[minor] Use Throwables.propagate?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11598092
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
@pimenas Thanks for the contribution! Merged to master `b259af4` and 1.7.x `10a1b23`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47819181
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -0,0 +1,6 @@
> +{
I always just take the JSON data and run it through [JSONlint](http://jsonlint.com/) because it formats everything nicely. Please add a newline after the last `}`, thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11598599
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1292](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1292/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47436941
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +@Extension(of = ServiceType.COMPUTE, namespace = ExtensionNamespaces.CONSOLES)
> +@RequestFilters(AuthenticateRequest.class)
> +public interface ConsolesAsyncApi {
> + /**
> + * @see ConsolesApi#getConsole
> + */
> + @Named("server:console")
> + @POST
> + @Path("/servers/{serverId}/action")
> + @SelectJson("console")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Produces(MediaType.APPLICATION_JSON)
> + @Fallback(MapHttp4xxCodesToExceptions.class)
> + @MapBinder(BindConsoleToJsonPayload.class)
> + ListenableFuture<? extends Console> getConsole(@PathParam("serverId") String serverId,
> + @PayloadParam("type") Console.Type type);
As stated in my previous comments we need a custom wrap value depending on the Console.Type value. As I see with @WrapWith we can only have one value.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13025530
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
@pimenas Looking forward to the updates!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47262043
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + switch (type) {
> + case NOVNC:
> + case XVPVNC:
> + action = "os-getVNCConsole";
> + break;
> + case SPICE_HTML5:
> + action = "os-getSPICEConsole";
> + break;
> + case RDP_HTML5:
> + action = "os-getRDPConsole";
> + break;
> + default:
> + throw new IllegalArgumentException("Invalid type: " + type);
> + }
> +
> + return bindToRequest(request, (Object) ImmutableMap.of(action, ImmutableSortedMap.copyOf(postParams)));
Through a second look, the casting here seems unnecessary. I'll remove this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13038421
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #790](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/790/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-42589997
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/spice_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
> +
> + assertEquals(api.getConsolesExtensionForZone("az-1.region-a.geo-1")
> + .get().getConsole(serverId, Console.Type.SPICE_HTML5).toString(),
> + new ParseSPICEConsoleTest().expected().toString());
The answers to your questions, is that I followed the style of existing tests (e.g. KeyPairApiExpectTest)
I'll remove this test completely once I'm sure about how to test using the mockwebserver style tests (see ConsolesApiMockTest.java).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13038708
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/spice_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
> +
> + assertEquals(api.getConsolesExtensionForZone("az-1.region-a.geo-1")
> + .get().getConsole(serverId, Console.Type.SPICE_HTML5).toString(),
> + new ParseSPICEConsoleTest().expected().toString());
> The answers to your questions, is that I followed the style of existing tests (e.g. KeyPairApiExpectTest)
OK...thanks for explaining!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13058969
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
This should re-trigger the build.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44108009
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1294](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1294/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44108022
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1056](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1056/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41883653
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #781](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/781/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-42099455
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Just uploaded my changes which include switching to URI for console url and enum for console type.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40316074
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> + if (type.equals(value.type)) {
> + return value;
> + }
> + }
> + return UNRECOGNIZED;
> + }
> + return null;
> + }
The wire logs will (if enabled) contain the actual value which resulted in UNRECOGNIZED.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13354426
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + return "/vnc_console.json";
> + }
> +
> + @Override
> + @SelectJson("console")
> + @Consumes(MediaType.APPLICATION_JSON)
> + public Console expected() {
> + try {
> + return Console
> + .builder()
> + .url(new URI("http://example.com:6080/vnc_auto.html?token=f9906a48-b71e-4f18"
> + + "-baca-c987da3ebdb3&title=dafa(75ecef58-3b8e-4659-ab3b-5501454188e9)"))
> + .type(Console.Type.NOVNC)
> + .build();
> + } catch (Exception e) {
> + throw new RuntimeException(e);
[minor] see above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11598127
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1222](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1222/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40318159
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
In previous revisions I had the following implementation:
```
private static final Map<String, Type> types;
static {
ImmutableMap.Builder<String, Type> builder = new ImmutableMap.Builder<String, Type>();
for (Type t : values()) {
builder.put(t.type(), t);
}
types = builder.build();
}
...
/**
* Used from jclouds builtin deserializer.
*/
public static Type fromValue(String type) {
return types.get(type);
}
```
but @zack-shoylev suggested to switch to make it like in the example, see previous related comments.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13038541
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
I pushed the changes, so now it should be ok.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13193496
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #939](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/939/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47440048
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1314](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1314/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44569868
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
In alignment with `valueOf`, perhaps name this `valueOfType` or `fromType`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11597608
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
I'll proceed with the fixes, and I would also like to replace `getRDPConsole`, `getSPICEConsole` and `getRDPConsole` with a single `getConsole`. Any objections on that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40431161
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1003](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1003/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39820125
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
Reopened #339.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#event-124552211
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
As per the discussion above, simply use `Type.fromValue(type)` here? That will fail if it's `null` or an invalid return value from the server, but it's OK to fail in that case.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13103814
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1136](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1136/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44089549
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
Thanks, @pimenas! Haven't looked at the content yet, just starting with some formatting things.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39799315
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> +@Extension(of = ServiceType.COMPUTE, namespace = ExtensionNamespaces.CONSOLES)
> +@RequestFilters(AuthenticateRequest.class)
> +public interface ConsolesAsyncApi {
> + /**
> + * @see ConsolesApi#getConsole
> + */
> + @Named("server:console")
> + @POST
> + @Path("/servers/{serverId}/action")
> + @SelectJson("console")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Produces(MediaType.APPLICATION_JSON)
> + @Fallback(MapHttp4xxCodesToExceptions.class)
> + @MapBinder(BindConsoleToJsonPayload.class)
> + ListenableFuture<? extends Console> getConsole(@PathParam("serverId") String serverId,
> + @PayloadParam("type") Console.Type type);
Ah I finally get it. This is unpleasant =\
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13028634
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Hi @jdaggett, @everett-toews!
I'll make the changes and add a "live" test as well. A question that I have
is if you think that the Console class,
is good as it is, or if for example I should change the url to URI type,
and make the type an enum. What do you think?
Epimenidis Voutsakis
Electronics & Computer Engineer
pimenas@gmail.com
+30 695 720 6307 (Mobile, Greece)
On Fri, Apr 11, 2014 at 11:02 PM, Everett Toews <no...@github.com>wrote:
> Good catch @jdaggett <https://github.com/jdaggett>!
>
> Jeremy is right. The Console API is an OpenStack extension and needs to be
> coded as such in jclouds. The particular call you're making is for the os-consoles
> extension<http://api.openstack.org/api-ref-compute-v2-ext.html#ext-os-consoles>
> .
>
> --
> Reply to this email directly or view it on GitHub<https://github.com/jclouds/jclouds/pull/339#issuecomment-40247575>
> .
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40275126
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +
> + /**
> + * Get SPICE Console
> + * @param serverId Server id
> + * @param type Only "spice-html5" is supported
> + * @return a Console object containing the console url and type.
> + */
> + Console getSPICEConsole(String serverId, Console.Type type);
> +
> + /**
> + * Get RDP Console
> + * @param serverId Server id
> + * @param type Only "rdp-html5" is supported
> + * @return a Console object containing the console url and type.
> + */
> + Console getRDPConsole(String serverId, Console.Type type);
Well I would actually prefer providing one `getConsole(String serverId, ConsoleType type)` and use the type argument to distinguish the actual call to be made to openstack. I believe also @jdaggett agrees with this below.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11613970
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +
> + public void testGetConsoleWhenResponseIs401NotAuthorized() throws Exception {
> + String serverId = "d3aae414-9dc9-46d1-a51b-e9b7071a84b7";
> + String type = "novnc";
> +
> + HttpRequest getConsole = HttpRequest
> + .builder()
> + .method("POST")
> + .addHeader("Accept", "application/json")
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11378285
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> @@ -270,7 +270,6 @@
> * @return A Map containing the collected values organized by key - value.
> * @Beta
> */
> - Optional<Map<String, String>> getDiagnostics(String id);
> -
> + Optional<Map<String, String>> getDiagnostics(String id);
Thanks for the cleanup, @pimenas ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11597963
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1160](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1160/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44373482
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + /**
> + * Get SPICE Console
> + * @param serverId Server id
> + * @param type Only "spice-html5" is supported
> + * @return a Console object containing the console url and type.
> + */
> + Console getSPICEConsole(String serverId, Console.Type type);
> +
> + /**
> + * Get RDP Console
> + * @param serverId Server id
> + * @param type Only "rdp-html5" is supported
> + * @return a Console object containing the console url and type.
> + */
> + Console getRDPConsole(String serverId, Console.Type type);
Hm...if each of these methods only accepts _some_ of the values of the `Type` enum, would it make sense to have three enums? In fast, since SPICE and RDP only support one valid value _anyway_, how about just using the enum for getVNCConsole?
And unless we expect the number of options to grow rapidly, would two methods `getNoVNCConsole` and `getXvpVNCConsole` (not sure about the correct capitalization here) be easier for users to work with?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11597880
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #825](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/825/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44113577
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #938](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/938/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47436652
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> + if (type.equals(value.type)) {
> + return value;
> + }
> + }
> + return UNRECOGNIZED;
> + }
> + return null;
> + }
> In the implementation you provide, you use the enum valueOf(), which cannot be overriden, in order to map the
> values in the constructors with the enum values.
Indeed. Maybe I was misunderstanding your code, but unless `equals` on Type is overridden `type.equals(value.type)` in the current implementation should have the same effect?
If, as @zack-shoylev suggests, we should support case-insensitive matching too, then `Type.valueOf` in my suggestion will of course need to be replaced something more like:
```
public static Type fromValue(String value) {
if (value == null) {
return null;
}
// Or compute the value in the Type class and expose a method lowercaseValues() there?
// Or indeed simply add a `caseInsensitiveValueOf(String value)` method on Type?
for (Type type : Type.values()) {
if (type.equalsIgnoreCase(value)) {
return value;
}
}
logger.warn("Unrecognized console type: %s", type); // or logger.debug or so?
return UNRECOGNIZED;
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13315742
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/spice_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
> +
> + assertEquals(api.getConsolesExtensionForZone("az-1.region-a.geo-1")
> + .get().getConsole(serverId, Console.Type.SPICE_HTML5).toString(),
> + new ParseSPICEConsoleTest().expected().toString());
It won't matter, although I use the same style ConsolesApiMockTest.java.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13194219
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1249](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1249/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41893260
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Hi all,
I'm currently proceeding with the changes, and I stumbled upon the switching to one `getConsole()` function.
In `ConsolesAsyncApi.java` the `@Payload` annotation needs an extra parameter, that is related to the console type parameter. Any ideas on how to pass the correct os-getVNCConsole, os-getRDPConsole or os-getSPICEConsole, depending on the value of the Console type?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41494267
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + Console.Type type = (Console.Type) postParams.get("type");
> +
> + switch (type) {
> + case NOVNC:
> + case XVPVNC:
> + action = "os-getVNCConsole";
> + break;
> + case SPICE_HTML5:
> + action = "os-getSPICEConsole";
> + break;
> + case RDP_HTML5:
> + action = "os-getRDPConsole";
> + break;
> + default:
> + throw new IllegalArgumentException("Invalid type: " + type);
> In my opinion however the binder is a more appropriate place
If this is the "first" place the input is actually processed then yes, I'd agree that this is the correct place. But then we shouldn't need this check in `fromValue` in the enum?
Thanks for explaining!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13100857
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #780](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/780/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41891565
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
:) You're all very welcome and thank you also!!! It's been a very learning experience for me.
@everett-toews indeed its [me](https://twitter.com/pimenas) on twitter :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47831452
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> + if (type.equals(value.type)) {
> + return value;
> + }
> + }
> + return UNRECOGNIZED;
> + }
> + return null;
> + }
I like your implementation, I think a `valueOfIgnoreCase()` is not needed here, and the log level I believe it should be warn() so that future extensions/changes can be noticed sooner.
The problem I have with this implementation is the logger variable. I cannot use the following code to inject a logger because it needs to be references from a static context, and I don't think the @Resource will work if I change the logger to static.
```
@Resource
private Logger logger = Logger.NULL;
```
By searching the code I found the following which uses the `java.util.logging.Logger` (code taken from VirtualMachineApiLiveTest.java)
```
private static final Logger logger = Logger.getAnonymousLogger();
```
So any suggestions on the logger, or should I remove it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13331588
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Hi!
I pushed the changes, I also added a mockstyle test, but I would like some feedback on this.
many thanks for all the help so far!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41887119
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/vnc_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
@pimenas @demobox Yes, MockWebServer style tests please! Take a look at this [example](
https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-swift/src/test/java/org/jclouds/openstack/swift/v1/features/ContainerApiMockTest.java).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11598451
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + *
> + * @author Epi Vou
> + */
> +@Singleton
> +public class BindConsoleToJsonPayload extends BindToJsonPayload {
> +
> + @Inject
> + public BindConsoleToJsonPayload(Json jsonBinder) {
> + super(jsonBinder);
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Map<String, Object> postParams) {
> + String action = null;
> +
> + Console.Type type = (Console.Type) postParams.get("type");
Is this needed? fromValue in the enumType in conjunction with toString should ensure GSON mapping happens automatically as per https://wiki.apache.org/jclouds/Enums
I am probably missing something?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r12868663
Re: [jclouds] openstack console (#339)
Posted by Everett Toews <no...@github.com>.
EPIC pull request for your first contribution to jclouds. Thank you so much for sticking with it!
Is this [you on Twitter](https://twitter.com/pimenas)? We'd love to give you a shoutout for the work you've done here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47824425
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> Hi @demobox! It's my first attempt to contribute!
I know! ;-) Glad to have you contributing! And you did better than almost all of us by noticing (and then fixing, too!) the Checkstyle warnings ;-)
As regards the code, I think @everett-toews, @zack-shoylev or @jdaggett would be best placed to comment.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39845273
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
I also like this solution.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13116837
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1143](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1143/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44175693
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +
> + Console.Type type = (Console.Type) postParams.get("type");
> +
> + switch (type) {
> + case NOVNC:
> + case XVPVNC:
> + action = "os-getVNCConsole";
> + break;
> + case SPICE_HTML5:
> + action = "os-getSPICEConsole";
> + break;
> + case RDP_HTML5:
> + action = "os-getRDPConsole";
> + break;
> + default:
> + throw new IllegalArgumentException("Invalid type: " + type);
I believe this can happen, if the someone pass a null value as the type argument of getConsole() function in ConsolesApi, or as you say if someone modifies the enum without updating here as well.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13038118
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
Closed #339.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#event-124551978
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #735](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/735/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39786658
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +@Test(groups = "live", testName = "ConsolesApiLiveTest")
> +public class ConsolesApiLiveTest extends BaseNovaApiLiveTest {
> +
> + @Test
> + public void testGetVNCConsole() {
> + for (String zoneId : api.getConfiguredZones()) {
> + Optional<? extends ConsolesApi> apiOption = api.getConsolesExtensionForZone(zoneId);
> + if (!apiOption.isPresent())
> + continue;
> +
> + ConsolesApi api = apiOption.get();
> + ServerApi serverApi = this.api.getServerApiForZone(zoneId);
> + Server server = createServerInZone(zoneId);
> + Console console = api.getConsole(server.getId(), Console.Type.NOVNC);
> + assertNotNull(console);
> + assertNotNull(console.getType());
I'll remove this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13038816
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> Is there anything else that needs to be done with this PR?
Looks like it might need a rebase..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-46995840
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1251](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1251/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-42099526
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1295](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1295/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47441977
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +
> + public void testGetConsoleWhenResponseIs404NotFound() throws Exception {
> + String serverId = "d3aae414-9dc9-46d1-a51b-e9b7071a84b7";
> + String type = "novnc";
> +
> + HttpRequest getConsole = HttpRequest
> + .builder()
> + .method("POST")
> + .addHeader("Accept", "application/json")
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11378283
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> +
> + Console.Type type = (Console.Type) postParams.get("type");
> +
> + switch (type) {
> + case NOVNC:
> + case XVPVNC:
> + action = "os-getVNCConsole";
> + break;
> + case SPICE_HTML5:
> + action = "os-getSPICEConsole";
> + break;
> + case RDP_HTML5:
> + action = "os-getRDPConsole";
> + break;
> + default:
> + throw new IllegalArgumentException("Invalid type: " + type);
When calling getConsole() the user can supply a valid Console.Type object or null. If null the binder will first process it (`fromValue()` is used later in the response) and will throw an IAE. If not null, then it can only be a valid Console.Type and thus `toString()` will serialize the enum. If all goes well we'll get a response object which will be deserialized in a Console object (containing a url, and a Console.Type). At this point is were fromValue() will be used to deserialize the response type to a Console.Type enum. If at this point the type returned from openstack is not valid, then maybe we should throw some exception which could be IAE since from a little search I did, I haven't found something more appropriate.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13103055
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1411](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1411/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47440846
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + switch (type) {
> + case NOVNC:
> + case XVPVNC:
> + action = "os-getVNCConsole";
> + break;
> + case SPICE_HTML5:
> + action = "os-getSPICEConsole";
> + break;
> + case RDP_HTML5:
> + action = "os-getRDPConsole";
> + break;
> + default:
> + throw new IllegalArgumentException("Invalid type: " + type);
> + }
> +
> + return bindToRequest(request, (Object) ImmutableMap.of(action, ImmutableSortedMap.copyOf(postParams)));
What is the cast required for? To ensure the correct overloaded method is called? In that case, assign as a variable?
```
Object paramsObject ImmutableMap.of(action, ImmutableSortedMap.copyOf(postParams));
return bindToRequest(request, paramsObject);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13035006
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1139](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1139/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44116661
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/spice_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
> +
> + assertEquals(api.getConsolesExtensionForZone("az-1.region-a.geo-1")
> + .get().getConsole(serverId, Console.Type.SPICE_HTML5).toString(),
> + new ParseSPICEConsoleTest().expected().toString());
> Personally I would prefer removing toString(), because then the test would also fail if Console.equals() method is
> not implemented correctly
Agreed. So let's remove the `toString` ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13100944
Re: [jclouds] openstack console (#339)
Posted by Ignasi Barrera <no...@github.com>.
Jclouds already has a built-in enum deserializer. It will use the `valueOf` method, but will fallback to a `fromValue` one if the former does not work. If the values in the json document don't match the enum names, the easiest way is to provide a proper `fromValue` method in the enum. Does this work for you?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40306683
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #941](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/941/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47440803
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1298](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1298/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44169306
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
See response above. An IAE in the binder seems the right thing, but then we should never get `null` here and don't need the check?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13100899
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> jclouds » jclouds #1061 UNSTABLE
Unrelated [test failure](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/org.apache.jclouds$jclouds-core/1061/testReport/junit/org.jclouds.util/Predicates2Test/testFalseOnExecutionException/)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-42105129
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #737](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/737/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39820440
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> @@ -0,0 +1,6 @@
> +{
[minor] Indenting in this and the other JSON files? I know we don't seem to have a good standard right now...@zack-shoylev, @jdaggett - what do you use?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11598184
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + * Server Console Connection structure.
> + *
> + * @author Epi Vou
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html" />
> + */
> +public class Console {
> + public enum Type {
> + NOVNC("novnc"),
> + XVPVNC("xvpvnc"),
> + SPICE_HTML5("spice-html5"),
> + RDP_HTML5("rdp-html5");
> +
> + private static final Map<String, Type> types;
> +
> + static {
> + ImmutableMap.Builder<String, Type> builder = new ImmutableMap.Builder<String, Type>();
You can define (in the enum) RDP_HTML5("rdp-html5"); and it should be matched (using case-insensitive matching if you follow the wiki pattern). The match would be done using the name string (the one in quotes).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13028647
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1293](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1293/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47439871
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1138](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1138/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44109316
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
I pushed the change in Console.Type enum suggested by @zack-shoylev in comments above.
Any suggestions on the ConsolesApiMockTest.java, ConsolesApiLiveTest.java?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44114247
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +import java.beans.ConstructorProperties;
> +
> +import com.google.common.base.Objects;
> +import com.google.common.base.Objects.ToStringHelper;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +
> +/**
> + * Connection information to connect to a server using VNC.
> + *
> + * @author Epi Vou
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html" />
> + */
> +public class Console {
> +
> + public static Builder<?> builder() {
[formatting] jclouds uses 3-space indents.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11371760
Re: [jclouds] openstack console (#339)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1077](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1077/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-42588496
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + */
> +@Test(groups = "live", testName = "ConsolesApiLiveTest")
> +public class ConsolesApiLiveTest extends BaseNovaApiLiveTest {
> +
> + @Test
> + public void testGetVNCConsole() {
> + for (String zoneId : api.getConfiguredZones()) {
> + Optional<? extends ConsolesApi> apiOption = api.getConsolesExtensionForZone(zoneId);
> + if (!apiOption.isPresent())
> + continue;
> +
> + ConsolesApi api = apiOption.get();
> + ServerApi serverApi = this.api.getServerApiForZone(zoneId);
> + Server server = createServerInZone(zoneId);
> + Console console = api.getConsole(server.getId(), Console.Type.NOVNC);
> + assertNotNull(console);
The test two lines below will fail if `console` is `null`, so personally I'd say we could remove this. But fine to keep it in if you like the explicit non-null check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13035082
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1311](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1311/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44375036
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1205](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1205/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39787585
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + /**
> + * Get SPICE Console
> + * @param serverId Server id
> + * @param type Only "spice-html5" is supported
> + * @return a Console object containing the console url and type.
> + */
> + Console getSPICEConsole(String serverId, Console.Type type);
> +
> + /**
> + * Get RDP Console
> + * @param serverId Server id
> + * @param type Only "rdp-html5" is supported
> + * @return a Console object containing the console url and type.
> + */
> + Console getRDPConsole(String serverId, Console.Type type);
> Well I would actually prefer providing one getConsole(String serverId, ConsoleType type)
That's also fine by me, especially if the methods actually all have the same response type.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11614448
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> + public void testGetConsoleWhenResponseIs401NotAuthorized() throws Exception {
> + String serverId = "d3aae414-9dc9-46d1-a51b-e9b7071a84b7";
> + String type = "novnc";
> +
> + HttpRequest getConsole = HttpRequest
> + .builder()
> + .method("POST")
> + .addHeader("Accept", "application/json")
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
[minor] indents
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11371881
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> + if (type.equals(value.type)) {
> + return value;
> + }
> + }
> + return UNRECOGNIZED;
> + }
> + return null;
> + }
- According to the documentation here: http://developer.openstack.org/api-ref-compute-v2-ext.html, it's not clarified if `type` cannot be null, actually the response is not documented at all, besides providing an example. Now according to the openstack source code, this field always gets a valid value.
- In the implementation you provide, you use the enum valueOf(), which cannot be overriden, in order to map the values in the constructors with the enum values.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13222639
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/vnc_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
The regions in keystoneAuthResponse.json are az-1.region-a.geo-1, az-2.region-a.geo-1 and az-3.region-a.geo-1. In the line you mention, zoneId becomes "az-1.region-a.geo-1", and passed as argument in the `novaApi.getConsolesExtensionForZone(zoneId)`. Nevertheless I have tried a valid zone instead of "RegionTwo" but I get the same exception always.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r12159303
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Hello! I've implemented all the suggestions, removed the expect style tests, and updated the MockWebStyle and live tests.
I also merged with the latest master branch, so it's ready for merging!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47437463
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.openstack.v2_0.ServiceType;
> +import org.jclouds.openstack.v2_0.services.Extension;
> +
> +import com.google.common.annotations.Beta;
> +
> +/**
> + * Provides synchronous access to Consoles.
> + * <p/>
> + *
> + * @see ConsoleAsyncApi
> + */
> +@Beta
> +@Extension(of = ServiceType.COMPUTE, namespace = ExtensionNamespaces.CONSOLES)
> +public interface ConsolesApi {
> + /**
> + * Get Console
"Gets a console" or "Gets the console"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13035035
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> +
> +/**
> + * Provides asynchronous access to Consoles via the REST API.
> + * <p/>
> + *
> + * @see ConsoleApi
> + * @author Epi Vou
> + * @see ExtensionAsyncApi
> + * @see <a href= "http://docs.openstack.org/api/openstack-compute/2/content/Extensions-d1e1444.html" />
> + * @see <a href="http://nova.openstack.org/api_ext" />
> + * @see <a href="http://api.openstack.org/api-ref-compute-v2-ext.html#ext-os-consoles" />
> + */
> +@Beta
> +@Extension(of = ServiceType.COMPUTE, namespace = ExtensionNamespaces.CONSOLES)
> +@RequestFilters(AuthenticateRequest.class)
> +public interface ConsolesAsyncApi {
> Still working through that, but for now, I would say keep the async interface.
Thanks for clarifying!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11599511
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
Reopened #339.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#event-124551979
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> + if (type.equals(value.type)) {
> + return value;
> + }
> + }
> + return UNRECOGNIZED;
> + }
> + return null;
> + }
I might be misunderstanding some of the previous discussion here?
* Can `type` here ever be `null` on a legitimate response from the server (cc @zack-shoylev?) If so, what should the response be? `null` or `UNRECOGNIZED`?
* Can `type` here ever be a non-null value that doesn't match the `Type` enum? From what I understand, that may indeed be possible (e.g. if the jclouds enum is out-of-date), and the result in that case should be `UNRECOGNIZED`
In that case, how about something like:
```
public static Type fromValue(String type) {
if (type == null) {
return null;
}
try {
return Type.valueOf(type);
} catch(IllegalArgumentException e) {
logger.warn("Unrecognized console type: " + type); // or logger.debug or so?
return UNRECOGNIZED;
}
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13215901
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
Mark the return value as `@Nullable`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13035014
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1299](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1299/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44176013
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #828](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/828/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44176161
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> + if (type.equals(value.type)) {
> + return value;
> + }
> + }
> + return UNRECOGNIZED;
> + }
> + return null;
> + }
@pimenas is correct.
Specifics: Personally I am very much in favor of supporting both null, unrecognized, and case-insensitive matching in the enums. This is mostly because it ensures that if undocumented behavior changes, jclouds still works. (Clearly documented nullability might have to be handled differently, for example).
I actually like the logger.warn on UNRECOGNIZED and I think I will add it to the wiki example.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13311734
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1221](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1221/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40291427
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
If I understand correctly, all the "real" tests only verify NOVNC consoles? Is there any way to test the other types? Also, I don't think I saw any tests for our enum type conversion logic...is that implicitly tested in the mock tests?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-44154466
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + }
> +
> + private final String type;
> +
> + Type(String type) {
> + this.type = type;
> + }
> +
> + public String type() {
> + return type;
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
fromValue() is used by the jclouds deserializer for the enums, so if I change this name, then json deserializing won't work. Should I leave it like this, or do something else?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r11606437
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
> + .addHeader("X-Auth-Token", authToken)
> + .payload(payloadFromStringWithContentType(
> + "{\"os-getVNCConsole\":{\"type\":\"novnc\"}}",
> + "application/json"))
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/"
> + + "servers/" + serverId + "/action")
> + .build();
> +
> + HttpResponse getConsoleResponse = HttpResponse.builder().statusCode(200)
> + .message("HTTP/1.1 200 OK")
> + .payload(payloadFromResourceWithContentType("/vnc_console.json",
> + "application/json; charset=UTF-8")).build();
> +
> + NovaApi api = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
> + responseWithKeystoneAccess, extensionsOfNovaRequest, extensionsOfNovaResponse,
> + getConsole, getConsoleResponse);
Hi,
Following the examples, I tried this:
```
@Test(groups = "unit", testName = "ConsolesApiMockTest")
public class ConsolesApiMockTest extends BaseOpenStackMockTest<NovaApi> {
public void getVNCConsole() throws Exception {
String serverId = "5f64fca7-879b-4173-bf9c-8fa88330a4dc";
MockWebServer server = mockOpenStackServer();
server.enqueue(addCommonHeaders(new MockResponse().setBody(stringFromResource("/keystoneAuthResponse.json"))));
server.enqueue(addCommonHeaders(new MockResponse().setBody(stringFromResource("/extension_list_full.json"))));
try {
NovaApi novaApi = api(server.getUrl("/").toString(), "openstack-nova");
String zoneId = getFirst(novaApi.getConfiguredZones(), "RegionTwo");
Optional<? extends ConsolesApi> consolesApi = novaApi.getConsolesExtensionForZone(zoneId);
...
} finally {
server.shutdown();
}
}
}
```
and I get an exception
```
org.jclouds.http.HttpResponseException: CacheLoader returned null for key [identity=jclouds:joe, credential=letmein]. connecting to GET https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/extensions HTTP/1.1
at org.jclouds.http.internal.BaseHttpCommandExecutorService.invoke(BaseHttpCommandExecutorService.java:162)
at org.jclouds.rest.internal.InvokeSyncToAsyncHttpMethod.invoke(InvokeSyncToAsyncHttpMethod.java:129)
...
```
which is caused in `novaApi.getConsolesExtensionForZone(zoneId)` because it's trying to actually connect to the publicURL for the specific zone. I don't know how to proceed from this point, so once more I ask for your help!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r12129059
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #752](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/752/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-40317937
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #779](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/779/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-41889823
Re: [jclouds] openstack console (#339)
Posted by Jeremy Daggett <no...@github.com>.
Closed #339.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#event-137559866
Re: [jclouds] openstack console (#339)
Posted by Zack Shoylev <no...@github.com>.
Closed #339.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#event-124552208
Re: [jclouds] openstack console (#339)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + /**
> + * Used from jclouds builtin deserializer.
> + */
> + public static Type fromValue(String type) {
> + if (type != null) {
> + for (Type value : Type.values()) {
> + if (type.equals(value.type)) {
> + return value;
> + }
> + }
> + return UNRECOGNIZED;
> + }
> + return null;
> + }
> FTR: Logger.html#getAnonymousLogger
"This factory method is primarily intended for use from applets." Oooo, it's really modern!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339/files#r13334443
Re: [jclouds] openstack console (#339)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1409](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1409/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-47436691
Re: [jclouds] openstack console (#339)
Posted by pimenas <no...@github.com>.
Hi @demobox! It's my first attempt to contribute!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/339#issuecomment-39817198