You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Christopher Dancy <no...@github.com> on 2014/12/11 22:06:22 UTC
[jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Initial commit for shipyard provider. My intent was to have the entire provider coded and in something like "good working shape" before I submitted a PR but @nacx strongly suggested I not do this.
This code base is a fork of the work @andreaturli is doing with docker so there may be more than a few similarities. Shipyard itself is essentially a docker manager allowing users to connect N number of engines (docker daemons) to a single source allowing you to work with them from a single location. A few points to note:
-There are literally no testers at this point. Working on this...
-Around 10 files or so are json that gets handed back from shipyard. Used for testing purposes and do not need to be looked over.
-The idea here is to implement the entirety of ComputeServiceAdapter. All methods except 'createNodeWithGroupIntoName' have been coded. Working on this...
-A lot of files still do not have apache headers. Working on this...
So with that said please take a peek and let me know what you think!
You can merge this Pull Request by running:
git pull https://github.com/cdancy/jclouds-labs JCLOUDS-782
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/116
-- Commit Summary --
* init for shipyard provider
-- File Changes --
M .gitignore (1)
A shipyard/.gitignore (1)
A shipyard/README.md (16)
A shipyard/pom.xml (162)
A shipyard/src/main/java/org/jclouds/shipyard/ShipyardApi.java (56)
A shipyard/src/main/java/org/jclouds/shipyard/ShipyardApiMetadata.java (99)
A shipyard/src/main/java/org/jclouds/shipyard/ShipyardProviderMetadata.java (76)
A shipyard/src/main/java/org/jclouds/shipyard/compute/config/ShipyardComputeServiceContextModule.java (73)
A shipyard/src/main/java/org/jclouds/shipyard/compute/functions/ContainerInfoToNodeMetadata.java (103)
A shipyard/src/main/java/org/jclouds/shipyard/compute/functions/EngineInfoToLocation.java (35)
A shipyard/src/main/java/org/jclouds/shipyard/compute/functions/ImageInfoToImage.java (71)
A shipyard/src/main/java/org/jclouds/shipyard/compute/functions/InjectLocationIntoImageInfo.java (37)
A shipyard/src/main/java/org/jclouds/shipyard/compute/functions/ShipyardStateToStatus.java (39)
A shipyard/src/main/java/org/jclouds/shipyard/compute/options/ShipyardTemplateOptions.java (508)
A shipyard/src/main/java/org/jclouds/shipyard/compute/strategy/ShipyardComputeServiceAdapter.java (277)
A shipyard/src/main/java/org/jclouds/shipyard/compute/strategy/ShipyardListNodesStrategy.java (42)
A shipyard/src/main/java/org/jclouds/shipyard/config/ShipyardHttpApiModule.java (38)
A shipyard/src/main/java/org/jclouds/shipyard/config/ShipyardParserModule.java (28)
A shipyard/src/main/java/org/jclouds/shipyard/config/ShipyardProperties.java (26)
A shipyard/src/main/java/org/jclouds/shipyard/domain/accounts/AccountInfo.java (28)
A shipyard/src/main/java/org/jclouds/shipyard/domain/accounts/CreateAccount.java (26)
A shipyard/src/main/java/org/jclouds/shipyard/domain/accounts/DeleteAccount.java (20)
A shipyard/src/main/java/org/jclouds/shipyard/domain/cluster/ClusterInfo.java (46)
A shipyard/src/main/java/org/jclouds/shipyard/domain/containers/ContainerImageInfo.java (48)
A shipyard/src/main/java/org/jclouds/shipyard/domain/containers/ContainerInfo.java (37)
A shipyard/src/main/java/org/jclouds/shipyard/domain/engines/AddEngine.java (29)
A shipyard/src/main/java/org/jclouds/shipyard/domain/engines/EngineInfo.java (21)
A shipyard/src/main/java/org/jclouds/shipyard/domain/engines/EngineSettingsInfo.java (30)
A shipyard/src/main/java/org/jclouds/shipyard/domain/events/EventInfo.java (38)
A shipyard/src/main/java/org/jclouds/shipyard/domain/images/ImageInfo.java (35)
A shipyard/src/main/java/org/jclouds/shipyard/domain/images/ImagePortsInfo.java (26)
A shipyard/src/main/java/org/jclouds/shipyard/domain/roles/CreateRole.java (20)
A shipyard/src/main/java/org/jclouds/shipyard/domain/roles/DeleteRole.java (20)
A shipyard/src/main/java/org/jclouds/shipyard/domain/roles/RoleInfo.java (20)
A shipyard/src/main/java/org/jclouds/shipyard/domain/servicekeys/CreateServiceKey.java (20)
A shipyard/src/main/java/org/jclouds/shipyard/domain/servicekeys/DeleteServiceKey.java (20)
A shipyard/src/main/java/org/jclouds/shipyard/domain/servicekeys/ServiceKeyInfo.java (21)
A shipyard/src/main/java/org/jclouds/shipyard/fallbacks/NullOn500.java (26)
A shipyard/src/main/java/org/jclouds/shipyard/features/AccountsApi.java (58)
A shipyard/src/main/java/org/jclouds/shipyard/features/ClusterApi.java (40)
A shipyard/src/main/java/org/jclouds/shipyard/features/ContainersApi.java (77)
A shipyard/src/main/java/org/jclouds/shipyard/features/EnginesApi.java (65)
A shipyard/src/main/java/org/jclouds/shipyard/features/EventsApi.java (42)
A shipyard/src/main/java/org/jclouds/shipyard/features/ImagesApi.java (28)
A shipyard/src/main/java/org/jclouds/shipyard/features/RolesApi.java (66)
A shipyard/src/main/java/org/jclouds/shipyard/features/ServiceKeysApi.java (60)
A shipyard/src/main/java/org/jclouds/shipyard/features/WebhookKeysApi.java (26)
A shipyard/src/main/java/org/jclouds/shipyard/filters/ServiceKeyAuthentication.java (56)
A shipyard/src/main/java/org/jclouds/shipyard/handlers/ShipyardErrorHandler.java (98)
A shipyard/src/test/java/org/jclouds/shipyard/DockerApiMetadataTest.java (48)
A shipyard/src/test/resources/accounts_create.json (58)
A shipyard/src/test/resources/accounts_delete.json (58)
A shipyard/src/test/resources/accounts_info.json (58)
A shipyard/src/test/resources/cluster_info.json (8)
A shipyard/src/test/resources/containers.json (206)
A shipyard/src/test/resources/engines.json (14)
A shipyard/src/test/resources/events.json (211)
A shipyard/src/test/resources/images.json (92)
A shipyard/src/test/resources/logback.xml (34)
A shipyard/src/test/resources/servicekeys.json (10)
A shipyard/src/test/resources/webhookkeys.json (7)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/116.patch
https://github.com/jclouds/jclouds-labs/pull/116.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +public interface ContainersApi {
> +
> + @Named("containers:list")
> + @GET
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<ContainerInfo> listContainers();
> +
> + @Named("containers:info")
> + @GET
> + @Path("/{id}")
> + ContainerInfo getContainer(@PathParam("id") String id);
> +
> + @Named("containers:logs")
> + @GET
> + @Path("/{id}/logs")
> + String getContainerLogs(@PathParam("id") String id);
Same comment about the length of the response. Worth changing to an InputStream or Reader, or just removing this method?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22195791
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> +import org.jclouds.shipyard.filters.ServiceKeyAuthentication;
> +
> +@Consumes(MediaType.APPLICATION_JSON)
> +@RequestFilters({ ServiceKeyAuthentication.class })
> +public interface ContainersApi {
> +
> + @Named("containers:list")
> + @GET
> + @Path("/api/containers")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<ContainerInfo> listContainers();
> +
> + @Named("containers:info")
> + @GET
> + @Path("/api/containers/{id}")
> + @Fallback(NullOn500.class)
This is done a few times throughout the API. Unfortunately when a "thing" can not be accessed or is not found Shipyard is throwing a 500. Not very intuitive on there part. I've brought the issue up with devs just not sure when it's going to be addressed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22189007
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<ShipyardApi, Builder> {
> +
> + protected Builder() {
> + super(ShipyardApi.class);
> + id("shipyard")
> + .name("Shipyard Remote Docker Management API")
> + .identityName("<shipyard-service-key>")
> + .credentialName("not used")
> + .documentation(URI.create("http://shipyard-project.com/docs/api/"))
> + .version("2.0.4")
> + .defaultEndpoint("https://127.0.0.1:8080")
> + .defaultProperties(ShipyardApiMetadata.defaultProperties())
> + .defaultModules(ImmutableSet.<Class<? extends Module>>of(
> + OkHttpCommandExecutorServiceModule.class,
Remove this from this list. The preferred way is to install it as part of the `ShipyardHttpApiModule`, which has already been done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22195556
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> + case 402:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 404:
> + if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
> + exception = new ResourceNotFoundException(message, exception);
> + }
> + break;
> + case 405:
> + exception = new IllegalArgumentException(message, exception);
> + break;
> + case 409:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 500:
> + exception = null;
This issue now appears to be a red-herring IMO. With the response now coming back as "null" upon resource not being found jclouds successfully returns this as a null object instead of throwing an error trying to convert it to json. I think we are safe moving forward with no needed Fallback implementation or returning of null on 500. I think this is as happy a path as we can expect.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22237334
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + <id>live</id>
> + <build>
> + <plugins>
> + <plugin>
> + <groupId>org.codehaus.mojo</groupId>
> + <artifactId>rat-maven-plugin</artifactId>
> + <configuration>
> + <excludes>
> + <exclude>.apt_generated/**/*</exclude>
> + <exclude>target/**/*</exclude>
> + <exclude>.settings/**/*</exclude>
> + <exclude>.project</exclude>
> + <exclude>.factorypath</exclude>
> + <exclude>.classpath</exclude>
> + <exclude>.gitignore</exclude>
> + </excludes>
Can you open a PR to the main repo to add the required exclusions to the `project/pom.xml`? Then we can rebase on top of that and have a clean pom.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21861699
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + case 402:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 404:
> + if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
> + exception = new ResourceNotFoundException(message, exception);
> + }
> + break;
> + case 405:
> + exception = new IllegalArgumentException(message, exception);
> + break;
> + case 409:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 500:
> + exception = null;
This doesn't seem to be correct.
Do the 500 errors provide some information in the response? Is there a way you can determine if you should propagate a `ResourceNotFoundException` or another more appropriate?
It does not seem right to just blindly ignore any 500 error. We wouldn't know if it is a *real* server failure or not.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22195987
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> + @GET
> + @Path("/api/containers")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<ContainerInfo> listContainers();
> +
> + @Named("containers:info")
> + @GET
> + @Path("/api/containers/{id}")
> + @Fallback(NullOn500.class)
> + ContainerInfo getContainer(@PathParam("id") String id);
> +
> + @Named("containers:logs")
> + @GET
> + @Path("/api/containers/{id}/logs")
> + @Fallback(NullOn500.class)
> + String getContainerLogs(@PathParam("id") String id);
This was added for completeness sake. Until it makes sense to have it I think removing this call would make the most sense.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22188964
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + properties.setProperty(Constants.PROPERTY_MAX_RETRIES, "5");
> + properties.setProperty(ShipyardProperties.SHIPYARD_CREDENTIAL, "<remote-service-key-given-by-shipyard-cli>");
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<ShipyardApi, Builder> {
> +
> + protected Builder() {
> + super(ShipyardApi.class);
> + id("shipyard")
> + .name("Shipyard Remote Docker Management API")
> + .identityName("<service-key>")
> + .credentialName(null)
This should *describe* the credential. If no credential is needed, just say something like "not used" or so, but don't leave this `null`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21861967
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + *
> + * @see <a href= "http://en.wikipedia.org/wiki/Basic_access_authentication" />
> + */
> +@Singleton
> +public class ServiceKeyAuthentication implements HttpRequestFilter {
> + private final Supplier<Credentials> creds;
> +
> + @Inject
> + public ServiceKeyAuthentication(@Provider Supplier<Credentials> creds) {
> + this.creds = checkNotNull(creds, "creds");
> + }
> +
> + @Override
> + public HttpRequest filter(HttpRequest request) throws HttpException {
> + Credentials currentCreds = checkNotNull(creds.get(), "credential supplier returned null");
> + if (currentCreds.identity == null || currentCreds.identity.contains(":")) {
Why this ':' check? If only alphanumeric characters are allowed, wouldn't a regexp make more sense to validate the identity?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21863271
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.shipyard.filters.ServiceKeyAuthentication;
> +
> +@Consumes(MediaType.APPLICATION_JSON)
> +@RequestFilters({ ServiceKeyAuthentication.class })
> +public interface ContainersApi {
> +
> + @Named("containers:list")
> + @GET
> + @Path("/api/containers")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<ContainerInfo> listContainers();
> +
> + @Named("containers:info")
> + @GET
> + @Path("/api/containers/{id}")
> + @Fallback(NullOn500.class)
When can a 500 error happen? Why returning `null` instead of failing?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862739
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> + case 402:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 404:
> + if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
> + exception = new ResourceNotFoundException(message, exception);
> + }
> + break;
> + case 405:
> + exception = new IllegalArgumentException(message, exception);
> + break;
> + case 409:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 500:
> + exception = null;
So this has actually been addressed in recent commits. The 500 is no longer returned but a one-liner "null", along with 200 response code, is sent instead. I've pasted the output just below. This is true for all requests where something specific is requested. Might this be a use-case for coding a custom fallback for methods/requests which expect something specific returned but get this "null" string?
> GET /api/engines/123 HTTP/1.1
> User-Agent: curl/7.37.1
> Host: sdrelnx150:8080
> Accept: */*
> X-Service-Key: Kxtie5Ujgs7SsqLzIgrDuwKJ9XU9yAuQwWri
>
< HTTP/1.1 200 OK
< Content-Type: application/json
< Date: Tue, 23 Dec 2014 13:22:28 GMT
< Content-Length: 5
<
null
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22214785
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> +import javax.ws.rs.GET;
> +import javax.ws.rs.Path;
> +import javax.ws.rs.core.MediaType;
> +
> +import org.jclouds.Fallbacks.EmptyListOnNotFoundOr404;
> +import org.jclouds.rest.annotations.EndpointParam;
> +import org.jclouds.rest.annotations.Fallback;
> +import org.jclouds.shipyard.domain.images.ImageInfo;
> +
> +@Consumes(MediaType.APPLICATION_JSON)
> +@Path("/images/json")
> +public interface ImagesApi {
> +
> + @Named("images:list")
> + @GET
> + @Fallback(EmptyListOnNotFoundOr404.class)
yes it should ;)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r25258503
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +import javax.ws.rs.core.MediaType;
> +
> +import org.jclouds.Fallbacks.EmptyListOnNotFoundOr404;
> +import org.jclouds.rest.annotations.Fallback;
> +import org.jclouds.rest.annotations.RequestFilters;
> +import org.jclouds.shipyard.domain.containers.ContainerInfo;
> +import org.jclouds.shipyard.fallbacks.NullOn500;
> +import org.jclouds.shipyard.filters.ServiceKeyAuthentication;
> +
> +@Consumes(MediaType.APPLICATION_JSON)
> +@RequestFilters({ ServiceKeyAuthentication.class })
> +public interface ContainersApi {
> +
> + @Named("containers:list")
> + @GET
> + @Path("/api/containers")
Extract this path annotation to the root API. The "delegating" APIs propagate the path to their children, so you just have to annotate the methods in child APIs with the remaining path segments.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862708
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @cdancy! This is a great starting PR. Code looks pretty good!
Once the initial comments are addressed, and before being able to merge it, unit tests must be added:
* MockWebserver tests for each Api method.
* Unit tests for the Authentication filter.
* Unit tests for the custom fallback class, if we keep it.
You can take a look at GCE or digital ocean for examples of MWS tests. For other unit tests you can just create what you need, or use EasyMock. Let us know if you need help!
Please, don't squash the changes until the PR is about to merge, at it is much easier to review just the new commits than going through the entire PR again :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-67080637
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> + @Override
> + public Builder toBuilder() {
> + return new Builder().fromApiMetadata(this);
> + }
> +
> + public ShipyardApiMetadata() {
> + this(new Builder());
> + }
> +
> + protected ShipyardApiMetadata(Builder builder) {
> + super(builder);
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + properties.setProperty(ShipyardProperties.SHIPYARD_CREDENTIAL, "<remote-service-key-given-by-shipyard-cli>");
It's not being used just a leftover artifact from cloning the docker project. Removing now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r23532536
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + public Builder toBuilder() {
> + return new Builder().fromApiMetadata(this);
> + }
> +
> + public ShipyardApiMetadata() {
> + this(new Builder());
> + }
> +
> + protected ShipyardApiMetadata(Builder builder) {
> + super(builder);
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + properties.setProperty(Constants.PROPERTY_MAX_RETRIES, "5");
> + properties.setProperty(ShipyardProperties.SHIPYARD_CREDENTIAL, "<remote-service-key-given-by-shipyard-cli>");
Is this a *good* value for this property? Will the Api work if users don't override it? Should this value be passed as the credential?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862022
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds</groupId>
> + <artifactId>jclouds-compute</artifactId>
> + <version>${project.version}</version>
> + </dependency>
> + <dependency>
> + <groupId>com.google.auto.value</groupId>
> + <artifactId>auto-value</artifactId>
> + <scope>provided</scope>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-slf4j</artifactId>
> + <version>${project.version}</version>
> + </dependency>
Don't force users to a concrete logging driver. Move it to the test scope if needed, or just remove the dependency.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21861494
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.shipyard.domain.images.ImagePortsInfo;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class ContainerImageInfo {
> +
> + @Nullable public abstract String name();
> +
> + @Nullable public abstract Map<String, String> environment();
> +
> + @Nullable public abstract List<String> entrypoint();
> +
> + public abstract String hostname();
> +
> + @Nullable public abstract List<ImagePortsInfo> bind_ports();
Also, nullable lists are usually a bad idea. Could you work that out in the create method so an empty list is returned instead of a null one? Or are these list also used to "post" information to the provider and they *must* be null?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862476
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.rest.AuthorizationException;
> +
> +import com.google.common.base.Supplier;
> +
> +/**
> + * Shipyard remote API authentication is made via the HTTP header 'X-Service-Key' which in turns
> + * has it's value as an encoded string (Shipyard-cli generates this for you).
> + *
> + * @see <a href= "http://en.wikipedia.org/wiki/Basic_access_authentication" />
> + */
> +@Singleton
> +public class ServiceKeyAuthentication implements HttpRequestFilter {
> + private final Supplier<Credentials> creds;
> +
> + @Inject
> + public ServiceKeyAuthentication(@Provider Supplier<Credentials> creds) {
Make the constructor package protected. This way you make sure it is only used by the Guice injector, which will already enforce the parameters to be present. Then you can also remove the `checkNotNull` check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21863193
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-75572002
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + @SerializedNames({ "name", "environment", "entrypoint", "hostname", "bind_ports", "volumes", "restart_policy", "publish", "network_mode" })
> + public static ContainerImageInfo create(String name, Map<String, String> environment, List<String> entryPoint,
> + String hostName, List<ImagePortsInfo> bindPorts,
> + List<String> volumes, Map<String, String> restartPolicy,
> + boolean publish, String networkMode) {
> +
> + if (environment == null)
> + environment = Maps.newHashMap();
> + if (entryPoint == null)
> + entryPoint = Lists.newArrayList();
> + if (bindPorts == null)
> + bindPorts = Lists.newArrayList();
> + if (volumes == null)
> + volumes = Lists.newArrayList();
> + if (restartPolicy == null)
> + restartPolicy = Maps.newHashMap();
Using inline ifs here would be more readable?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22195599
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> + *
> + * @see <a href= "http://en.wikipedia.org/wiki/Basic_access_authentication" />
> + */
> +@Singleton
> +public class ServiceKeyAuthentication implements HttpRequestFilter {
> + private final Supplier<Credentials> creds;
> +
> + @Inject
> + public ServiceKeyAuthentication(@Provider Supplier<Credentials> creds) {
> + this.creds = checkNotNull(creds, "creds");
> + }
> +
> + @Override
> + public HttpRequest filter(HttpRequest request) throws HttpException {
> + Credentials currentCreds = checkNotNull(creds.get(), "credential supplier returned null");
> + if (currentCreds.identity == null || currentCreds.identity.contains(":")) {
So this is my ... sad ... attempt at making sure one does not pass in OpenStack like credentials (e.g. <some.project>:<some.username>). It was mainly to code against something I had found here, where we use OpenStack and Shipyard, in that we provide a higher level API on top of jclouds and found that some users, who were familiar with OpenStack, passed in the same identity format for shipyard. It should have been addressed in the higher level API but I did so here in my efforts to understand how jclouds works. I'll remove.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22216617
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> + case 402:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 404:
> + if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
> + exception = new ResourceNotFoundException(message, exception);
> + }
> + break;
> + case 405:
> + exception = new IllegalArgumentException(message, exception);
> + break;
> + case 409:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 500:
> + exception = null;
I'm thinking it may be best to remove this entire ErrorHandler all together and fall back to what I was doing it before, NullOn500, which is done on a per-request basis. The 500 here is the sticky point and doing this for the general case is not a good solution. The other solution is not great either but it's better than this. I say we go with that, however unfortunate, until the devs address the issue at some further date.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22196971
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.shipyard;
> +
> +import org.jclouds.rest.annotations.Delegate;
> +import org.jclouds.shipyard.features.ContainersApi;
> +import org.jclouds.shipyard.features.EnginesApi;
> +import org.jclouds.shipyard.features.ImagesApi;
> +
> +import java.io.Closeable;
> +
> +public interface ShipyardApi extends Closeable {
> +
> + @Delegate
> + ContainersApi getContainersApi();
Prefer a naming like `containersApi()` for delegate apis.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21861836
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> + break;
> + case 405:
> + exception = new IllegalArgumentException(message, exception);
> + break;
> + case 409:
> + exception = new IllegalStateException(message, exception);
> + break;
> + }
> + } finally {
> + closeQuietly(response.getPayload());
> + command.setException(exception);
> + }
> + }
> +
> + public String parseMessage(HttpResponse response) {
Make this method private
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21863472
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + @Override
> + public Builder toBuilder() {
> + return new Builder().fromApiMetadata(this);
> + }
> +
> + public ShipyardApiMetadata() {
> + this(new Builder());
> + }
> +
> + protected ShipyardApiMetadata(Builder builder) {
> + super(builder);
> + }
> +
> + public static Properties defaultProperties() {
> + Properties properties = BaseHttpApiMetadata.defaultProperties();
> + properties.setProperty(ShipyardProperties.SHIPYARD_CREDENTIAL, "<remote-service-key-given-by-shipyard-cli>");
What is this property used for?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22746743
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
Just a couple comments, and the code looks good. Once the tests mentioned in my previous comment are added, this will be good to go. Thanks @cdancy!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-69407759
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> + void deleteContainer(@PathParam("id") String id);
> +
> + @Named("containers:stop")
> + @GET
> + @Path("/api/containers/{id}/stop")
> + void stopContainer(@PathParam("id") String id);
> +
> + @Named("containers:restart")
> + @GET
> + @Path("/api/containers/{id}/restart")
> + void restartContainer(@PathParam("id") String id);
> +
> + @Named("containers:scale")
> + @GET
> + @Path("/api/containers/{id}/scale?count={count}")
> + void scaleContainer(@PathParam("id") String id, @PathParam("count") int count);
Also added for completeness sake. Will remove as it's not being used and is not necessary for jclouds compute abstraction.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22189115
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + <id>live</id>
> + <build>
> + <plugins>
> + <plugin>
> + <groupId>org.codehaus.mojo</groupId>
> + <artifactId>rat-maven-plugin</artifactId>
> + <configuration>
> + <excludes>
> + <exclude>.apt_generated/**/*</exclude>
> + <exclude>target/**/*</exclude>
> + <exclude>.settings/**/*</exclude>
> + <exclude>.project</exclude>
> + <exclude>.factorypath</exclude>
> + <exclude>.classpath</exclude>
> + <exclude>.gitignore</exclude>
> + </excludes>
I've just seen that the labs parent pom.xml already has the rat-plugin configuration, so just move the missing excludes there and remove the config from this pom.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21865298
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + * limitations under the License.
> + */
> +package org.jclouds.shipyard.domain.images;
> +
> +import java.util.List;
> +
> +import org.jclouds.domain.Location;
> +import org.jclouds.javax.annotation.Nullable;
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class ImageInfo {
> +
> + public abstract String Created();
Don't use capitalised method names.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862519
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + case 402:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 404:
> + if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
> + exception = new ResourceNotFoundException(message, exception);
> + }
> + break;
> + case 405:
> + exception = new IllegalArgumentException(message, exception);
> + break;
> + case 409:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 500:
> + exception = null;
Blindly converting 500 responses into *correct* responses is wrong, whether we do that on a generic handler or in each method. Do those 500 responses hace *any* information of what is going on, in the headers, body or wherever? We should use and parse that to do the right thing.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22197193
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<ShipyardApi, Builder> {
> +
> + protected Builder() {
> + super(ShipyardApi.class);
> + id("shipyard")
> + .name("Shipyard Remote Docker Management API")
> + .identityName("<service-key>")
> + .credentialName(null)
> + .documentation(URI.create("http://shipyard-project.com/docs/api/"))
> + .version("2.0.4")
> + .defaultEndpoint("https://127.0.0.1:8080")
> + .defaultProperties(ShipyardApiMetadata.defaultProperties())
> + .view(typeToken(ComputeServiceContext.class))
> + .defaultModules(ImmutableSet.<Class<? extends Module>>of(
> + OkHttpCommandExecutorServiceModule.class,
Same as commented above. Remove this module if possible. Users will have the choice of configuring it when creating the context.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862066
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> + exception = new AuthorizationException(message, exception);
> + break;
> + case 402:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 404:
> + if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
> + exception = new ResourceNotFoundException(message, exception);
> + }
> + break;
> + case 405:
> + exception = new IllegalArgumentException(message, exception);
> + break;
> + case 409:
> + exception = new IllegalStateException(message, exception);
> + break;
Good point and yes it would
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22189960
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
The squashed commit removes the .gitignore and the pom.xml file (causing the build failure). Can you fix that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-75572501
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + return properties;
> + }
> +
> + public static class Builder extends BaseHttpApiMetadata.Builder<ShipyardApi, Builder> {
> +
> + protected Builder() {
> + super(ShipyardApi.class);
> + id("shipyard")
> + .name("Shipyard Remote Docker Management API")
> + .identityName("<service-key>")
> + .credentialName(null)
> + .documentation(URI.create("http://shipyard-project.com/docs/api/"))
> + .version("2.0.4")
> + .defaultEndpoint("https://127.0.0.1:8080")
> + .defaultProperties(ShipyardApiMetadata.defaultProperties())
> + .view(typeToken(ComputeServiceContext.class))
This has to be removed until the ComputeService is implemented.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21863553
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
This is a good start with the Mock tests. But there are a couple things you have to address in all them:
* Add tests for failed responses (404, etc), for all methods where you explicitly define a `@Fallback`. Mock tests also have to verify that the fallback takes place when needed.
* In all tests that procude a body, you also need to verify that the generated body is the right one.
* In the `assertSent` method, add a check for the authentication header, to make sure the filter configured in each API is behaving properly too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-72627675
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + exception = new AuthorizationException(message, exception);
> + break;
> + case 402:
> + exception = new IllegalStateException(message, exception);
> + break;
> + case 404:
> + if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
> + exception = new ResourceNotFoundException(message, exception);
> + }
> + break;
> + case 405:
> + exception = new IllegalArgumentException(message, exception);
> + break;
> + case 409:
> + exception = new IllegalStateException(message, exception);
> + break;
Should it be better to handle the 500 errors here instead of having the null fallback?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21863461
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
@nacx rebased against master. All is working well now ;)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-75597979
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
Great! I'll wait for that and have a look once the build is green again :) Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-67895679
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +# Shipyard as a local cloud provider
> +
> +##Setup
> +TODO
> +
> +#How it works
> +TODO
> +
> +##Components
> +TODO
> +
> +#Notes:
> +TODO
> +
> +#Troubleshooting
> +TODO
Add some content, or remove the file :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21861327
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
@nacx thoughts on latest commit? Want to get the Ok with this latest round so that I can then push live tests so we can close this PR and work on the next set of commits for fleshing out Shipyard API (in a subsequent PR of course).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-72209930
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.shipyard.config;
> +
> +public class ShipyardProperties {
> +
> + // shipyard remote api access key
> + public static final String SHIPYARD_CREDENTIAL = "shipyard.credential";
> +
> + public static final String SHIPYARD_ID = "shipyard";
If this property is not used, just remove it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862106
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + <artifactId>mockwebserver</artifactId>
> + <scope>test</scope>
> + <exclusions>
> + <!-- Already provided by jclouds-sshj -->
> + <exclusion>
> + <groupId>org.bouncycastle</groupId>
> + <artifactId>bcprov-jdk15on</artifactId>
> + </exclusion>
> + </exclusions>
> + </dependency>
> + <dependency>
> + <groupId>org.jboss.shrinkwrap</groupId>
> + <artifactId>shrinkwrap-depchain</artifactId>
> + <version>1.2.2</version>
> + <type>pom</type>
> + <scope>test</scope>
What is this dependency used for?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21861587
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
Merged to master as [02bc515d](https://git1-us-west.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=02bc515d). Thanks @cdancy!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-75785235
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.shipyard.config;
> +
> +public class ShipyardProperties {
> +
> + private ShipyardProperties(){};
> +
> + // shipyard remote api access key
> + public static final String SHIPYARD_CREDENTIAL = "shipyard.credential";
Doing so now
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r25258482
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.shipyard.config;
> +
> +public class ShipyardProperties {
> +
> + // shipyard remote api access key
> + public static final String SHIPYARD_CREDENTIAL = "shipyard.credential";
> +
> + public static final String SHIPYARD_ID = "shipyard";
Also, configure this class to it can not be instantiated or subclassed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862128
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + @GET
> + @Path("/api/containers")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<ContainerInfo> listContainers();
> +
> + @Named("containers:info")
> + @GET
> + @Path("/api/containers/{id}")
> + @Fallback(NullOn500.class)
> + ContainerInfo getContainer(@PathParam("id") String id);
> +
> + @Named("containers:logs")
> + @GET
> + @Path("/api/containers/{id}/logs")
> + @Fallback(NullOn500.class)
> + String getContainerLogs(@PathParam("id") String id);
That's even better :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22191281
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + <plugin>
> + <groupId>org.apache.maven.plugins</groupId>
> + <artifactId>maven-surefire-plugin</artifactId>
> + <executions>
> + <execution>
> + <id>integration</id>
> + <phase>integration-test</phase>
> + <goals>
> + <goal>test</goal>
> + </goals>
> + <configuration>
> + <threadCount>1</threadCount>
> + <systemPropertyVariables>
> + <test.shipyard.endpoint>${test.shipyard.endpoint}</test.shipyard.endpoint>
> + <test.shipyard.api-version>${test.shipyard.api-version}</test.shipyard.api-version>
> + <test.shipyard.credential>${test.shipyard.identity}</test.shipyard.credential>
Change to `test.shipyard.identity`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21861731
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> +package org.jclouds.shipyard;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +import static org.testng.Assert.assertTrue;
> +
> +import org.jclouds.apis.ApiMetadata;
> +import org.jclouds.apis.Apis;
> +import org.jclouds.compute.internal.BaseComputeServiceApiMetadataTest;
> +import org.testng.annotations.Test;
> +
> +/**
> + * Unit tests for the {@link ShipyardApiMetadata} class.
> + */
> +@Test(groups = "unit", testName = "ShipyardApiMetadataTest")
> +public class ShipyardApiMetadataTest extends BaseComputeServiceApiMetadataTest {
This class is currently not adding any value, or doing anything useful for that matter, so I'm going to remove it until this initial round of checks is deemed Ok. Once the code looks good I'll go back and add in the testers.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22192437
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + <scope>provided</scope>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-slf4j</artifactId>
> + <version>${project.version}</version>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-okhttp</artifactId>
> + <version>${project.version}</version>
> + </dependency>
> + <dependency>
> + <groupId>ch.qos.logback</groupId>
> + <artifactId>logback-classic</artifactId>
> + </dependency>
Apply the same criteria than the one used for the slf4j driver dependency.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21861609
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Nullable public abstract String name();
> +
> + public abstract Map<String, String> environment();
> +
> + public abstract List<String> entryPoint();
> +
> + public abstract String hostName();
> +
> + public abstract List<ImagePortsInfo> bindPorts();
> +
> + public abstract List<String> volumes();
> +
> + public abstract Map<String, String> restartPolicy();
> +
> + @Nullable public abstract boolean publish();
nit: primitive types can't be null
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22195650
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + @GET
> + @Path("/api/containers")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<ContainerInfo> listContainers();
> +
> + @Named("containers:info")
> + @GET
> + @Path("/api/containers/{id}")
> + @Fallback(NullOn500.class)
> + ContainerInfo getContainer(@PathParam("id") String id);
> +
> + @Named("containers:logs")
> + @GET
> + @Path("/api/containers/{id}/logs")
> + @Fallback(NullOn500.class)
> + String getContainerLogs(@PathParam("id") String id);
How long can these logs be? Would it make sense to return an `InputStream` or a `Reader`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862784
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +import javax.ws.rs.GET;
> +import javax.ws.rs.Path;
> +import javax.ws.rs.core.MediaType;
> +
> +import org.jclouds.Fallbacks.EmptyListOnNotFoundOr404;
> +import org.jclouds.rest.annotations.EndpointParam;
> +import org.jclouds.rest.annotations.Fallback;
> +import org.jclouds.shipyard.domain.images.ImageInfo;
> +
> +@Consumes(MediaType.APPLICATION_JSON)
> +@Path("/images/json")
> +public interface ImagesApi {
> +
> + @Named("images:list")
> + @GET
> + @Fallback(EmptyListOnNotFoundOr404.class)
Should this fallback be removed too, according to the other apis?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r25257901
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + </dependency>
> + <dependency>
> + <groupId>com.google.auto.value</groupId>
> + <artifactId>auto-value</artifactId>
> + <scope>provided</scope>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-slf4j</artifactId>
> + <version>${project.version}</version>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-okhttp</artifactId>
> + <version>${project.version}</version>
> + </dependency>
Ok *sigh* YAATS (yet another API that sucks). Then make sure you override the `configure` and install the OkHttp module there to make sure the driver is always loaded.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22191218
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> +import org.jclouds.rest.AuthorizationException;
> +
> +import com.google.common.base.Supplier;
> +
> +/**
> + * Shipyard remote API authentication is made via the HTTP header 'X-Service-Key' which in turns
> + * has it's value as an encoded string (Shipyard-cli generates this for you).
> + *
> + * @see <a href= "http://en.wikipedia.org/wiki/Basic_access_authentication" />
> + */
> +@Singleton
> +public class ServiceKeyAuthentication implements HttpRequestFilter {
> + private final Supplier<Credentials> creds;
> +
> + @Inject
> + public ServiceKeyAuthentication(@Provider Supplier<Credentials> creds) {
Ahh very good and will do.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22189247
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @cdancy! And thanks for submitting the PR before having the entire provider :)
I'd like to suggest, though, that you reduce the scope of the PR. If you have a look at other similar pull requests ([Orion BlobStore](https://github.com/jclouds/jclouds-labs/pull/45), [Profitbricks](https://github.com/jclouds/jclouds-labs/pull/72) or [vSphere](https://github.com/jclouds/jclouds-labs/pull/61#issuecomment-60512184)), you'll see that they're still open or closed, but they never reached a state where they could be merged. That is because it is extremely difficult to address design and structural things when you have a patch with thousands of lines; it is very hard to cleanup everything if the individual building blocks are not ok.
I really want to avoid making the same mistakes and help you focus in what matters and what will allow you to write the provider in a way that is more likely to be merged soon, so I'd suggest the following "action plan":
* [ ] Drop the entire compute package from this PR.
* [ ] Drop all APIs except one (choose the one you prefer), and the model classes that are not directly used by that API.
* [ ] Drop the ProviderMetadata and leave only the ApiMetadata.
This will allow us to:
* Discuss the provider design and address structural things at the beginning so you can just build the rest of features on top of that.
* Have all core configuration properly coded and tested (error handlers, authentication, custom parsers, etc).
* Have a clean API in a good state to be merged, including mock and live tests.
It could seem that this approach would slow down the development of the provider, but again, take a look at the example PRs at the beginning of the comment. Having a clean starting point will help you get at speed, and adding small features will help you have the provider built sooner. This will also allow us to understand better the provider, and be able to help build it in an iterative manner.
Does this work for you?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-66702106
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
> + </dependency>
> + <dependency>
> + <groupId>com.google.auto.value</groupId>
> + <artifactId>auto-value</artifactId>
> + <scope>provided</scope>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-slf4j</artifactId>
> + <version>${project.version}</version>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-okhttp</artifactId>
> + <version>${project.version}</version>
> + </dependency>
It is mandatory due to requirement of having a body as part of DELETE request. Let me know if there is some other way I should be going out about things.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22187556
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + </dependency>
> + <dependency>
> + <groupId>com.google.auto.value</groupId>
> + <artifactId>auto-value</artifactId>
> + <scope>provided</scope>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-slf4j</artifactId>
> + <version>${project.version}</version>
> + </dependency>
> + <dependency>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-okhttp</artifactId>
> + <version>${project.version}</version>
> + </dependency>
Same here. Is OkHttp mandatory for this provider? If not, just remove it and let users choose the HTTP driver.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21861542
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +@AutoValue
> +public abstract class ContainerImageInfo {
> +
> + @Nullable public abstract String name();
> +
> + @Nullable public abstract Map<String, String> environment();
> +
> + @Nullable public abstract List<String> entrypoint();
> +
> + public abstract String hostname();
> +
> + @Nullable public abstract List<ImagePortsInfo> bind_ports();
> +
> + @Nullable public abstract List<String> volumes();
> +
> + @Nullable public abstract Map<String, String> restart_policy();
Same here. Apply the camel-case naming for accessors in all domain model classes.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862229
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + *
> + * @see <a href= "http://en.wikipedia.org/wiki/Basic_access_authentication" />
> + */
> +@Singleton
> +public class ServiceKeyAuthentication implements HttpRequestFilter {
> + private final Supplier<Credentials> creds;
> +
> + @Inject
> + public ServiceKeyAuthentication(@Provider Supplier<Credentials> creds) {
> + this.creds = checkNotNull(creds, "creds");
> + }
> +
> + @Override
> + public HttpRequest filter(HttpRequest request) throws HttpException {
> + Credentials currentCreds = checkNotNull(creds.get(), "credential supplier returned null");
> + if (currentCreds.identity == null || currentCreds.identity.contains(":")) {
Is really the ':' character the only one forbidden?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22195932
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.shipyard.config;
> +
> +public class ShipyardProperties {
> +
> + private ShipyardProperties(){};
> +
> + // shipyard remote api access key
> + public static final String SHIPYARD_CREDENTIAL = "shipyard.credential";
Remove this property (and the class) if unused.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r25257727
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
Very good. Been on vacation, not to mention a new baby, and just getting back into the swing of things. I'll look into getting those tests submitted now that this initial set of files is in good shape.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-71467534
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +package org.jclouds.shipyard;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +import static org.testng.Assert.assertTrue;
> +
> +import org.jclouds.apis.ApiMetadata;
> +import org.jclouds.apis.Apis;
> +import org.jclouds.compute.internal.BaseComputeServiceApiMetadataTest;
> +import org.testng.annotations.Test;
> +
> +/**
> + * Unit tests for the {@link ShipyardApiMetadata} class.
> + */
> +@Test(groups = "unit", testName = "ShipyardApiMetadataTest")
> +public class ShipyardApiMetadataTest extends BaseComputeServiceApiMetadataTest {
Change parent class to `BaseApiMetadataTest` until the ComputeService is implemented.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21863619
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.location.Provider;
> +import org.jclouds.rest.AuthorizationException;
> +
> +import com.google.common.base.Supplier;
> +
> +/**
> + * Shipyard remote API authentication is made via the HTTP header 'X-Service-Key' which in turns
> + * has it's value as an encoded string (Shipyard-cli generates this for you).
> + *
> + */
> +@Singleton
> +public class ServiceKeyAuthentication implements HttpRequestFilter {
> + private final Supplier<Credentials> creds;
> +
> + @Inject
> + protected ServiceKeyAuthentication(@Provider Supplier<Credentials> creds) {
Make all Guice constructors package private by removing the method modifier. Just:
```java
@Inject
ServiceKeyAuthentication(@Provider Supplier<Credentials> creds) {
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22195861
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +package org.jclouds.shipyard;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +import static org.testng.Assert.assertTrue;
> +
> +import org.jclouds.apis.ApiMetadata;
> +import org.jclouds.apis.Apis;
> +import org.jclouds.compute.internal.BaseComputeServiceApiMetadataTest;
> +import org.testng.annotations.Test;
> +
> +/**
> + * Unit tests for the {@link ShipyardApiMetadata} class.
> + */
> +@Test(groups = "unit", testName = "ShipyardApiMetadataTest")
> +public class ShipyardApiMetadataTest extends BaseComputeServiceApiMetadataTest {
In fact it is. It is checking that the service loader can load the metadata class. That is, it is making sure when someone calls `ContextBuilder.newBuilder("shipyard")` there won't be ugly exceptions. I'd keep the test.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r22192596
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
Thanks @nacx and yes it does. I've trimmed the file count down from 60 to 24. Entire compute package has been removed. All features (minus ContainersApi, ImagesApi, and EnginesApi) have been removed. You noted to keep only a single feature, which I chose to be ContainersApi as that is the real work-horse here, but that is interrelated with ImagesApi and EnginesApi (both of which are small) so I kept them around if only to keep compilation happy on my end.
All apache headers have been added.
All checkstyle violations have been fixed.
All the domain classes, for each of the above features, are standard AutoValue stuff.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-66787706
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
Closed #116.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#event-241014934
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +import javax.inject.Singleton;
> +
> +import org.jclouds.domain.Credentials;
> +import org.jclouds.http.HttpException;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.http.HttpRequestFilter;
> +import org.jclouds.location.Provider;
> +import org.jclouds.rest.AuthorizationException;
> +
> +import com.google.common.base.Supplier;
> +
> +/**
> + * Shipyard remote API authentication is made via the HTTP header 'X-Service-Key' which in turns
> + * has it's value as an encoded string (Shipyard-cli generates this for you).
> + *
> + * @see <a href= "http://en.wikipedia.org/wiki/Basic_access_authentication" />
This is not Basic Auth. Remove this link.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21863074
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.shipyard.domain.images.ImagePortsInfo;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class ContainerImageInfo {
> +
> + @Nullable public abstract String name();
> +
> + @Nullable public abstract Map<String, String> environment();
> +
> + @Nullable public abstract List<String> entrypoint();
> +
> + public abstract String hostname();
> +
> + @Nullable public abstract List<ImagePortsInfo> bind_ports();
Prefer a camel-case naming. Change to `bindPorts`. The `@SerializedNames` annotation should make deserialisation work.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21862193
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
Just two minors left. Nice job @cdancy! Can you fix them and squash again? Then I'll merge the PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-75772204
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Ignasi Barrera <no...@github.com>.
> + void deleteContainer(@PathParam("id") String id);
> +
> + @Named("containers:stop")
> + @GET
> + @Path("/api/containers/{id}/stop")
> + void stopContainer(@PathParam("id") String id);
> +
> + @Named("containers:restart")
> + @GET
> + @Path("/api/containers/{id}/restart")
> + void restartContainer(@PathParam("id") String id);
> +
> + @Named("containers:scale")
> + @GET
> + @Path("/api/containers/{id}/scale?count={count}")
> + void scaleContainer(@PathParam("id") String id, @PathParam("count") int count);
Prefer using `@QueryParam("count")` and remove it from the path, if possible.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116/files#r21863002
Re: [jclouds-labs] JCLOUDS-782: init for shipyard provider (#116)
Posted by Christopher Dancy <no...@github.com>.
@nacx I've believe I've addressed all issues you noted with latest commit. Let me add that tester I removed back in.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/116#issuecomment-67895294