You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by jclarke-ire <no...@github.com> on 2017/03/23 10:49:29 UTC
[jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and get
account API implementation (#375)
- Adding structure for Dimension Data CloudControl API JClouds implementation
- Adding single API for user account retrieval, with mock and live tests
- Adding HTTP request filter that adds organization value to API request paths
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/375
-- Commit Summary --
* - Adding structure for Dimension Data CloudControl API JClouds implementation
-- File Changes --
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/DimensionDataCloudControlApi.java (28)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/DimensionDataCloudControlApiMetadata.java (84)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/DimensionDataCloudControlProviderMetadata.java (75)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/DimensionDataCloudControlComputeServiceAdapter.java (124)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/config/DimensionDataComputeServiceContextModule.java (61)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/config/DimensionDataComputeServiceDependenciesModule.java (52)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/functions/BaseImageToHardware.java (63)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/functions/BaseImageToImage.java (44)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/functions/DatacenterToLocation.java (44)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/functions/ServerWithNatRuleToNodeMetadata.java (59)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/suppliers/OrganisationIdForAccount.java (51)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/config/DimensionDataCloudControlHttpApiModule.java (45)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/config/DimensionDataCloudControlParserModule.java (34)
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/Account.java (139)
R dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/BaseImage.java (47)
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/PaginatedCollection.java (3)
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/Server.java (6)
D dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/Status.java (84)
R dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/internal/ServerWithExternalIp.java (29)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/features/AccountApi.java (40)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/filters/OrganisationIdFilter.java (63)
A dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/features/AccountApiLiveTest.java (53)
A dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/features/AccountApiMockTest.java (67)
A dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/filters/OrganisationIdFilterTest.java (57)
A dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/internal/BaseDimensionDataCloudControlApiLiveTest.java (58)
A dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/internal/BaseDimensionDataCloudControlMockTest.java (129)
A dimensiondata/src/test/resources/account.json (26)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/375.patch
https://github.com/jclouds/jclouds-labs/pull/375.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by John Clarke <no...@github.com>.
jawnclarke commented on this pull request.
> +
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+import org.jclouds.apis.ApiMetadata;
+import org.jclouds.apis.BaseApiLiveTest;
+import org.jclouds.dimensiondata.cloudcontrol.DimensionDataCloudControlApi;
+import org.jclouds.dimensiondata.cloudcontrol.DimensionDataCloudControlApiMetadata;
+import org.jclouds.sshj.config.SshjSshClientModule;
+import org.testng.annotations.Test;
+
+import java.util.Properties;
+
+@Test(groups = "live")
+public class BaseDimensionDataCloudControlApiLiveTest extends BaseApiLiveTest<DimensionDataCloudControlApi> {
+
+ protected String ORG_ID = System
Thanks, Trevor! I've removed it.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#discussion_r107739492
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Trevor Flanagan <no...@github.com>.
trevorflanagan commented on this pull request.
> +
+import com.google.inject.Scopes;
+import org.jclouds.dimensiondata.cloudcontrol.DimensionDataCloudControlApi;
+import org.jclouds.location.config.LocationModule;
+import org.jclouds.location.suppliers.ImplicitLocationSupplier;
+import org.jclouds.location.suppliers.implicit.OnlyLocationOrFirstZone;
+import org.jclouds.rest.ConfiguresHttpApi;
+import org.jclouds.rest.config.HttpApiModule;
+
+@ConfiguresHttpApi
+public class DimensionDataCloudControlHttpApiModule extends HttpApiModule<DimensionDataCloudControlApi> {
+
+ @Override
+ protected void bindErrorHandlers() {
+ //TODO Implement Method
+ }
@nacx I intend on now retaining this method with the binding for the DimensionDataCloudControlErrorHandler
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#discussion_r107867301
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by John Clarke <no...@github.com>.
jawnclarke commented on this pull request.
> + assertNotNull(account);
+ assertEquals("devlab1-online-vendor-devuser1", account.userName());
+ assertEquals("Donal Lunny", account.fullName());
+ assertEquals("Donal", account.firstName());
+ assertEquals("Lunny", account.lastName());
+ assertEquals("devlab1-online-vendor-devuser1@opsource.net", account.emailAddress());
+ assertThat(account.roles().contains(Account.RoleType.create("storage")));
+ assertThat(account.roles().contains(Account.RoleType.create("primary administrator")));
+ assertEquals("1", account.phone().countryCode());
+ assertEquals("1234567", account.phone().number());
+ assertEquals("6ac1e746-b1ea-4da5-a24e-caf1a978789d", account.organization().id());
+ assertEquals("Eng Customer of DevLab1-homed online sign-up Vendor", account.organization().name());
+ assertEquals("DevLab1Geo1", account.organization().homeGeoName());
+ assertEquals("apidevlab1.opsourcecloud.net", account.organization().homeGeoApiHost());
+ assertEquals("DEVLAB1GEO1", account.organization().homeGeoId());
+ assertEquals("NORMAL", account.state());
Good to know this approach going forward. Thanks @nacx .
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#discussion_r107874347
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @jawnclarke!
Mind squashing the commits into a single one so I can cleanly merge this?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#issuecomment-289005127
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Trevor Flanagan <no...@github.com>.
trevorflanagan commented on this pull request.
> +import org.jclouds.location.suppliers.implicit.OnlyLocationOrFirstZone;
+import org.jclouds.rest.ConfiguresHttpApi;
+import org.jclouds.rest.config.HttpApiModule;
+
+@ConfiguresHttpApi
+public class DimensionDataCloudControlHttpApiModule extends HttpApiModule<DimensionDataCloudControlApi> {
+
+ @Override
+ protected void bindErrorHandlers() {
+ //TODO Implement Method
+ }
+
+ @Override
+ protected void bindRetryHandlers() {
+ //TODO Implement Method
+ }
@nacx I intend on now retaining this method with the binding for the `DimensionDataCloudControlErrorHandler`
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#discussion_r107867252
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + properties.setProperty(Constants.PROPERTY_CONNECTION_TIMEOUT, "1200000"); // 15 minutes
+ return properties;
+ }
+
+ public static class Builder extends BaseHttpApiMetadata.Builder<DimensionDataCloudControlApi, Builder> {
+
+ protected Builder() {
+ id("dimensiondata-cloudcontrol").name("DimensionData CloudControl API").identityName("user name")
+ .credentialName("user password")
+ .documentation(URI.create("http://www.dimensiondata.com/en-US/Solutions/Cloud"))
+ .defaultEndpoint("https://api-REGION.dimensiondata.com/").version("2.4")
+ .defaultProperties(DimensionDataCloudControlApiMetadata.defaultProperties())
+ .view(typeToken(ComputeServiceContext.class)).defaultModules(
+ ImmutableSet.<Class<? extends Module>>builder().add(DimensionDataCloudControlHttpApiModule.class)
+ .add(DimensionDataCloudControlParserModule.class)
+ .add(DimensionDataComputeServiceContextModule.class).build());
That's right.That's right. Let's remove the entire "compute" package (moving the org id supplier elsewhere if it is needed).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#discussion_r107870895
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Trevor Flanagan <no...@github.com>.
@trevorflanagan pushed 7 commits.
80cfa73 Adding back the default constructor as well as re-ordering the variable declarations.
dbf8392 Assert that AUTHORIZATION header is sent. Minor refactors and javadoc.
c54ff2f Improvements to OrganisationIdFilter based on the pull request comments.
0acfc8d Reducing the amount of code required for review on this pull request as well as adding some Error Handling for Dimension Data error responses.
b2966c9 Removing server from assertSent method.
b9688ba Improvements to OrganisationIdFilterTest, OrganisationIdForAccount and BaseDimensionDataCloudControlMockTest based on the pull request comments.
287e689 Adding back the singleThreaded=true as it is breaking the build.
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375/files/6702146f192adcfb5a0dece268fa6b3ada4d0fa1..287e689d5677213e98e3bbe5bd5ce1ffae7f4aab
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Ignasi Barrera <no...@github.com>.
nacx requested changes on this pull request.
Many thanks @jawnclarke!
I've added several comments. The PR looks good, but let's keep it simple to set the right foundation so we can easily add more APIs and build stuff on top of this without having to revisit it.
Figuring out the base stuff is the difficult part, and things will move faster once this is merged.
> + @Override
+ public Builder toBuilder() {
+ return new Builder().fromApiMetadata(this);
+ }
+
+ public DimensionDataCloudControlApiMetadata() {
+ this(new Builder());
+ }
+
+ protected DimensionDataCloudControlApiMetadata(Builder builder) {
+ super(builder);
+ }
+
+ public static Properties defaultProperties() {
+ Properties properties = BaseHttpApiMetadata.defaultProperties();
+ properties.setProperty(Constants.PROPERTY_CONNECTION_TIMEOUT, "1200000"); // 15 minutes
Can we avoid setting this? The connection timeout configures the time to get a connection opened, not the time required to read all the data. It looks excessive. Let's better remove this default and let users configure it as needed.
> + properties.setProperty(Constants.PROPERTY_CONNECTION_TIMEOUT, "1200000"); // 15 minutes
+ return properties;
+ }
+
+ public static class Builder extends BaseHttpApiMetadata.Builder<DimensionDataCloudControlApi, Builder> {
+
+ protected Builder() {
+ id("dimensiondata-cloudcontrol").name("DimensionData CloudControl API").identityName("user name")
+ .credentialName("user password")
+ .documentation(URI.create("http://www.dimensiondata.com/en-US/Solutions/Cloud"))
+ .defaultEndpoint("https://api-REGION.dimensiondata.com/").version("2.4")
+ .defaultProperties(DimensionDataCloudControlApiMetadata.defaultProperties())
+ .view(typeToken(ComputeServiceContext.class)).defaultModules(
+ ImmutableSet.<Class<? extends Module>>builder().add(DimensionDataCloudControlHttpApiModule.class)
+ .add(DimensionDataCloudControlParserModule.class)
+ .add(DimensionDataComputeServiceContextModule.class).build());
For now let's remove the ComputeServiceAdapter module, the implementation and all other transformation functions. They are not implemented and return `null` and placeholders, and provide no value and just add complexity to the code and the PR. Let's remove all that stuff and start with a clean API to set a good foundation for the provider.
That will help and allow further PRs to be easier and faster.
> +
+ @Override
+ protected void bindErrorHandlers() {
+ //TODO Implement Method
+ }
+
+ @Override
+ protected void bindRetryHandlers() {
+ //TODO Implement Method
+ }
+
+ @Override
+ protected void installLocations() {
+ install(new LocationModule());
+ bind(ImplicitLocationSupplier.class).to(OnlyLocationOrFirstZone.class).in(Scopes.SINGLETON);
+ }
Don't override this yet. Let's remove it for now and we'll add it back (if it turns needed) when implementing the compute service adapter.
> +import org.jclouds.location.suppliers.implicit.OnlyLocationOrFirstZone;
+import org.jclouds.rest.ConfiguresHttpApi;
+import org.jclouds.rest.config.HttpApiModule;
+
+@ConfiguresHttpApi
+public class DimensionDataCloudControlHttpApiModule extends HttpApiModule<DimensionDataCloudControlApi> {
+
+ @Override
+ protected void bindErrorHandlers() {
+ //TODO Implement Method
+ }
+
+ @Override
+ protected void bindRetryHandlers() {
+ //TODO Implement Method
+ }
You can remove this, as jclouds provides sane default retry handlers.
> +
+import com.google.inject.Scopes;
+import org.jclouds.dimensiondata.cloudcontrol.DimensionDataCloudControlApi;
+import org.jclouds.location.config.LocationModule;
+import org.jclouds.location.suppliers.ImplicitLocationSupplier;
+import org.jclouds.location.suppliers.implicit.OnlyLocationOrFirstZone;
+import org.jclouds.rest.ConfiguresHttpApi;
+import org.jclouds.rest.config.HttpApiModule;
+
+@ConfiguresHttpApi
+public class DimensionDataCloudControlHttpApiModule extends HttpApiModule<DimensionDataCloudControlApi> {
+
+ @Override
+ protected void bindErrorHandlers() {
+ //TODO Implement Method
+ }
Remove this method if you're not defining any custom error handler.
> + * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.config;
+
+import com.google.inject.AbstractModule;
+import com.google.inject.name.Names;
+import org.jclouds.Constants;
+import org.jclouds.json.config.GsonModule;
+
+import java.util.Collections;
+
+public class DimensionDataCloudControlParserModule extends AbstractModule {
+
+ @Override
+ protected void configure() {
+ Names.bindProperties(binder(), Collections.singletonMap(Constants.PROPERTY_PRETTY_PRINT_PAYLOADS, "true"));
There is no need to bind this. It is already bound to the context and users will be able to configure this property when creating it. Let's remove this line.
>
- Account() {
+ @Nullable
+ public abstract AccountPhoneNumber phone();
+
+ public abstract AccountOrganization organization();
+
+ @SerializedNames({ "userName", "fullName", "firstName", "lastName", "emailAddress", "roles", "phone", "department",
+ "customDefined1", "customDefined2", "organization", "state" })
+ public static Account create(String userName, String fullName, String firstName, String lastName,
+ String emailAddress, List<RoleType> roles, AccountPhoneNumber phone, String department, String customDefined1,
+ String customDefined2, AccountOrganization organization, String state) {
+ return builder().userName(userName).fullName(fullName).firstName(firstName).lastName(lastName)
+ .emailAddress(emailAddress)
+ .roles(roles == null ? ImmutableList.<RoleType>of() : ImmutableList.copyOf(roles)).phone(phone)
Just pass the list here as it comes from the parameter, and move the immutability logic to the builder following [this pattern](https://github.com/jclouds/jclouds-labs/blob/master/dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/Backup.java#L53-L62).
>
- Account() {
Why has this been removed? Providing a non-visible default constructor is a best-practice to enforce using the builders. Let's add the constructors back.
>
- Account() {
+ @Nullable
+ public abstract AccountPhoneNumber phone();
+
+ public abstract AccountOrganization organization();
+
+ @SerializedNames({ "userName", "fullName", "firstName", "lastName", "emailAddress", "roles", "phone", "department",
+ "customDefined1", "customDefined2", "organization", "state" })
jclouds makes some assumptions when serializing and deserializing. The order of the parameters in this annotation **must be the same** than the order in which the variables are declared in the class. Please, reorder accordingly.
> +import org.jclouds.http.HttpRequestFilter;
+
+import java.util.List;
+
+/**
+ * Accepts requests and modifies the endpoint path so that it is injected with the organisation id.
+ * Handles both oec and caas based URLs.
+ */
+@Singleton
+public class OrganisationIdFilter implements HttpRequestFilter {
+
+ private static final int ORGANIZATION_ID_INDEX = 3;
+ private final Supplier<String> organisationIdSupplier;
+
+ @Inject
+ public OrganisationIdFilter(@Memoized Supplier<String> organisationIdSupplier) {
Remove the `public` modifier. It is a best-practice in Guice to make all injection constructors visible just to the default scope.
> +
+ // to have the compute service adapter override default locations
+ install(new LocationsFromComputeServiceAdapterModule<ServerWithExternalIp, BaseImage, BaseImage, Datacenter>() {
+ });
+
+ super.configure();
+ }
+
+ @Provides
+ @Singleton
+ @Memoized
+ public final Supplier<String> getOrganisationIdForAccount(AtomicReference<AuthorizationException> authException,
+ @Named(PROPERTY_SESSION_INTERVAL) long seconds, DimensionDataCloudControlApi api) {
+ return MemoizedRetryOnTimeOutButNotOnAuthorizationExceptionSupplier
+ .create(authException, new OrganisationIdForAccount(api), seconds, TimeUnit.SECONDS);
+ }
Better move this supplier to the HttpApi module, since it is generic for the provider APIs, not specifically about the compute abstraction.
> +import org.jclouds.logging.Logger;
+
+import javax.annotation.Resource;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+@Singleton
+public class OrganisationIdForAccount implements Supplier<String> {
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final AccountApi api;
+
+ @Inject
+ public OrganisationIdForAccount(DimensionDataCloudControlApi api) {
Remove the `public` modifier.
> +import com.google.inject.name.Named;
+import org.jclouds.compute.reference.ComputeServiceConstants;
+import org.jclouds.dimensiondata.cloudcontrol.DimensionDataCloudControlApi;
+import org.jclouds.dimensiondata.cloudcontrol.domain.Account;
+import org.jclouds.dimensiondata.cloudcontrol.features.AccountApi;
+import org.jclouds.logging.Logger;
+
+import javax.annotation.Resource;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+@Singleton
+public class OrganisationIdForAccount implements Supplier<String> {
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
better remove if unused.
> +
+ private static final int ORGANIZATION_ID_INDEX = 3;
+ private final Supplier<String> organisationIdSupplier;
+
+ @Inject
+ public OrganisationIdFilter(@Memoized Supplier<String> organisationIdSupplier) {
+ this.organisationIdSupplier = organisationIdSupplier;
+ }
+
+ @Override
+ public HttpRequest filter(HttpRequest request) throws HttpException {
+ return request.toBuilder().replacePath(injectOrganisationId(request.getEndpoint().getPath())).build();
+ }
+
+ @VisibleForTesting
+ public String injectOrganisationId(String path) {
Don't make it public if it is just visible for testing. Remove the public modifier and leave it in default scope.
> + assertNotNull(account);
+ assertEquals("devlab1-online-vendor-devuser1", account.userName());
+ assertEquals("Donal Lunny", account.fullName());
+ assertEquals("Donal", account.firstName());
+ assertEquals("Lunny", account.lastName());
+ assertEquals("devlab1-online-vendor-devuser1@opsource.net", account.emailAddress());
+ assertThat(account.roles().contains(Account.RoleType.create("storage")));
+ assertThat(account.roles().contains(Account.RoleType.create("primary administrator")));
+ assertEquals("1", account.phone().countryCode());
+ assertEquals("1234567", account.phone().number());
+ assertEquals("6ac1e746-b1ea-4da5-a24e-caf1a978789d", account.organization().id());
+ assertEquals("Eng Customer of DevLab1-homed online sign-up Vendor", account.organization().name());
+ assertEquals("DevLab1Geo1", account.organization().homeGeoName());
+ assertEquals("apidevlab1.opsourcecloud.net", account.organization().homeGeoApiHost());
+ assertEquals("DEVLAB1GEO1", account.organization().homeGeoId());
+ assertEquals("NORMAL", account.state());
Values seem to be of a particular user. Change them for `notNull` asserts or something that can be run by anyone.
> + * 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.dimensiondata.cloudcontrol.features;
+
+import org.jclouds.dimensiondata.cloudcontrol.domain.Account;
+import org.jclouds.dimensiondata.cloudcontrol.internal.BaseDimensionDataCloudControlApiLiveTest;
+import org.testng.annotations.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.AssertJUnit.assertEquals;
+
+@Test(groups = "live", testName = "AccountApiLiveTest", singleThreaded = true)
Looks like it does not need to be single-threaded.
> +import org.testng.annotations.Test;
+
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.AssertJUnit.assertEquals;
+
+@Test(groups = "unit", testName = "AccountApiMockTest", singleThreaded = true)
+public class AccountApiMockTest extends BaseDimensionDataCloudControlMockTest {
+
+ @BeforeMethod
+ @Override
+ public void start() throws IOException {
+ super.start();
+ }
No need to override this.
> + assertEquals("Donal", account.firstName());
+ assertEquals("Lunny", account.lastName());
+ assertEquals("devlab1-online-vendor-devuser1@opsource.net", account.emailAddress());
+ assertThat(account.roles().contains(Account.RoleType.create("storage")));
+ assertThat(account.roles().contains(Account.RoleType.create("primary administrator")));
+ assertEquals("1", account.phone().countryCode());
+ assertEquals("1234567", account.phone().number());
+ assertEquals("Marketing", account.department());
+ assertEquals("custom defined 1", account.customDefined1());
+ assertEquals("custom defined 2", account.customDefined2());
+ assertEquals("6ac1e746-b1ea-4da5-a24e-caf1a978789d", account.organization().id());
+ assertEquals("Eng Customer of DevLab1-homed online sign-up Vendor", account.organization().name());
+ assertEquals("DevLab1Geo1", account.organization().homeGeoName());
+ assertEquals("apidevlab1.opsourcecloud.net", account.organization().homeGeoApiHost());
+ assertEquals("DEVLAB1GEO1", account.organization().homeGeoId());
+ assertEquals("NORMAL", account.state());
There is no need to verify the response deserialization in mock tests, so you can remove all these asserts. We just want to make sure the generated requests are the expected ones.
That method uses basic authentication (via its filter), so let's modify the `assertSent` method in the base mock test class to verify that the authentication header is sent too.
> + assertThat(account.roles().contains(Account.RoleType.create("primary administrator")));
+ assertEquals("1", account.phone().countryCode());
+ assertEquals("1234567", account.phone().number());
+ assertEquals("Marketing", account.department());
+ assertEquals("custom defined 1", account.customDefined1());
+ assertEquals("custom defined 2", account.customDefined2());
+ assertEquals("6ac1e746-b1ea-4da5-a24e-caf1a978789d", account.organization().id());
+ assertEquals("Eng Customer of DevLab1-homed online sign-up Vendor", account.organization().name());
+ assertEquals("DevLab1Geo1", account.organization().homeGeoName());
+ assertEquals("apidevlab1.opsourcecloud.net", account.organization().homeGeoApiHost());
+ assertEquals("DEVLAB1GEO1", account.organization().homeGeoId());
+ assertEquals("NORMAL", account.state());
+
+ assertSent(server, "GET", "/caas/2.4/user/myUser");
+ }
+
Add another test method that simulates a 404 response, to verify that the method returns `null`.
Since you are defining an explicit `@Fallback` in the api method, we'd better verify it behaves as expected on the affected responses.
> + * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.filters;
+
+import com.google.common.base.Supplier;
+import org.jclouds.dimensiondata.cloudcontrol.compute.suppliers.OrganisationIdForAccount;
+import org.jclouds.dimensiondata.cloudcontrol.internal.BaseDimensionDataCloudControlMockTest;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+
+import static org.testng.Assert.assertEquals;
+
+@Test(groups = "unit", testName = "OrganisationIdFilterTest", singleThreaded = true)
+public class OrganisationIdFilterTest extends BaseDimensionDataCloudControlMockTest {
There is no need for this test to be single-threaded nor to extend the base mock test class. You don't need to actually start a mock web server to test this class.
> +import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+
+import static org.testng.Assert.assertEquals;
+
+@Test(groups = "unit", testName = "OrganisationIdFilterTest", singleThreaded = true)
+public class OrganisationIdFilterTest extends BaseDimensionDataCloudControlMockTest {
+
+ @BeforeMethod
+ @Override
+ public void start() throws IOException {
+ super.start();
+ server.enqueue(xmlResponse("/account.json"));
+ }
Also remove this.
> +
+import static org.testng.Assert.assertEquals;
+
+@Test(groups = "unit", testName = "OrganisationIdFilterTest", singleThreaded = true)
+public class OrganisationIdFilterTest extends BaseDimensionDataCloudControlMockTest {
+
+ @BeforeMethod
+ @Override
+ public void start() throws IOException {
+ super.start();
+ server.enqueue(xmlResponse("/account.json"));
+ }
+
+ @Test
+ public void testCaasUrl() {
+ Supplier<String> orgIdSupplier = new OrganisationIdForAccount(api);
You can just return a mock supplier for this test: `Suppliers.ofInstance("6ac1e746-b1ea-4da5-a24e-caf1a978789d/")`.
This way you don't need the overhead of starting a mock server, creating the context, etc.
> + public void testCaasUrl() {
+ Supplier<String> orgIdSupplier = new OrganisationIdForAccount(api);
+ String expectedPath = "/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/0896551e-4fe3-4450-a627-ad5548e7e83a?clone=trevor-test2&desc=trevor-description2";
+ String updatedPath = new OrganisationIdFilter(orgIdSupplier).injectOrganisationId(
+ "/caas/2.4/server/0896551e-4fe3-4450-a627-ad5548e7e83a?clone=trevor-test2&desc=trevor-description2");
+ assertEquals(updatedPath, expectedPath);
+ }
+
+ @Test
+ public void testDontAddIfAlreadyPresent() {
+ Supplier<String> orgIdSupplier = new OrganisationIdForAccount(api);
+ String expectedPath = "/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/0896551e-4fe3-4450-a627-ad5548e7e83a";
+ String updatedPath = new OrganisationIdFilter(orgIdSupplier).injectOrganisationId(
+ "/caas/2.4/6ac1e746-b1ea-4da5-a24e-caf1a978789d/server/0896551e-4fe3-4450-a627-ad5548e7e83a");
+ assertEquals(updatedPath, expectedPath);
+ }
Also add a test to verify the behavior when the path segments are < 3 ?
> +@Test(groups = "live")
+public class BaseDimensionDataCloudControlApiLiveTest extends BaseApiLiveTest<DimensionDataCloudControlApi> {
+
+ public BaseDimensionDataCloudControlApiLiveTest() {
+ provider = "dimensiondata-cloudcontrol";
+ }
+
+ @Override
+ protected ApiMetadata createApiMetadata() {
+ return new DimensionDataCloudControlApiMetadata();
+
+ }
+
+ @Override
+ protected Iterable<Module> setupModules() {
+ return ImmutableSet.<Module>of(getLoggingModule(), new SshjSshClientModule());
The SSH module is only needed for the compute service tests, so you could remove this override method.
> + protected ApiMetadata createApiMetadata() {
+ return new DimensionDataCloudControlApiMetadata();
+
+ }
+
+ @Override
+ protected Iterable<Module> setupModules() {
+ return ImmutableSet.<Module>of(getLoggingModule(), new SshjSshClientModule());
+ }
+
+ @Override
+ protected Properties setupProperties() {
+ Properties overrides = super.setupProperties();
+ overrides.setProperty("jclouds.ssh.retry-auth", "true");
+ return overrides;
+ }
SSH config is only needed for compute tests, so you should be able to remove this method too.
> +
+ protected Properties overrides() {
+ return new Properties();
+ }
+
+ protected String url(String path) {
+ return server.getUrl(path).toString();
+ }
+
+ protected MockResponse jsonResponse(String resource) {
+ return new MockResponse().addHeader("Content-Type", "application/json").setBody(stringFromResource(resource));
+ }
+
+ protected MockResponse xmlResponse(String resource) {
+ return new MockResponse().addHeader("Content-Type", "application/xml").setBody(stringFromResource(resource));
+ }
Remove this? Looks like we're just consuming a json api.
> + return new MockResponse().addHeader("Content-Type", "application/json").setBody(stringFromResource(resource));
+ }
+
+ protected MockResponse xmlResponse(String resource) {
+ return new MockResponse().addHeader("Content-Type", "application/xml").setBody(stringFromResource(resource));
+ }
+
+ protected MockResponse responseUnexpectedError() {
+ return new MockResponse().setResponseCode(400).setStatus("HTTP/1.1 400 Bad Request")
+ .setBody("content: [{\"operation\":\"OPERATION\",\"responseCode\":\"UNEXPECTED_ERROR\"}]");
+ }
+
+ protected MockResponse responseResourceNotFound() {
+ return new MockResponse().setResponseCode(400).setStatus("HTTP/1.1 400 Bad Request")
+ .setBody("content: [{\"operation\":\"OPERATION\",\"responseCode\":\"RESOURCE_NOT_FOUND\"}]");
+ }
Hmmm, upon a 400 response, is this what the DimensionData API returns? Do clients need to actually read the response body to really know the cause of the failure?
In any case, it looks like we need a custom error handler to propagate the right exception upon failed requests. jclouds by default throws `HttpResponseExceptions`, but there are other exceptions that better represent the different response codes:
* IllegalArgumentException - for 400 Bad Request or 409 Conflict.
* ResourceNotFoundException - for 404 Not Found.
* AuthorizationException - for 403 Unauthorized.
Those can be handled by creating a `DimensionDataCloudControlErrorHandler` (you can take [the Packet one](https://github.com/jclouds/jclouds-labs/blob/master/packet/src/main/java/org/jclouds/packet/handlers/PacketErrorHandler.java) as an example) and declaring it in the http api module (as in [here](https://github.com/jclouds/jclouds-labs/blob/master/packet/src/main/java/org/jclouds/packet/config/PacketHttpApiModule.java#L48-L53)).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#pullrequestreview-28841227
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + assertNotNull(account);
+ assertEquals("devlab1-online-vendor-devuser1", account.userName());
+ assertEquals("Donal Lunny", account.fullName());
+ assertEquals("Donal", account.firstName());
+ assertEquals("Lunny", account.lastName());
+ assertEquals("devlab1-online-vendor-devuser1@opsource.net", account.emailAddress());
+ assertThat(account.roles().contains(Account.RoleType.create("storage")));
+ assertThat(account.roles().contains(Account.RoleType.create("primary administrator")));
+ assertEquals("1", account.phone().countryCode());
+ assertEquals("1234567", account.phone().number());
+ assertEquals("6ac1e746-b1ea-4da5-a24e-caf1a978789d", account.organization().id());
+ assertEquals("Eng Customer of DevLab1-homed online sign-up Vendor", account.organization().name());
+ assertEquals("DevLab1Geo1", account.organization().homeGeoName());
+ assertEquals("apidevlab1.opsourcecloud.net", account.organization().homeGeoApiHost());
+ assertEquals("DEVLAB1GEO1", account.organization().homeGeoId());
+ assertEquals("NORMAL", account.state());
We have 2 kinds ot tests in jclouds: mock and live tests.
Mock tests verify that we generate the requests we are expected to generate. One could think about them as "jclouds generates exactly what the API docs say".
Live tests are integration tests that perform real requests to the target cloud (and may cost money!). The idea is that anyone with an account should be able to run them to have the real status of the APIs, configure them in the CI to keep track of the changes on the provider API, etc.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#discussion_r107876525
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Ignasi Barrera <no...@github.com>.
nacx approved this pull request.
>
public class DimensionDataCloudControlParserModule extends AbstractModule {
@Override
protected void configure() {
- Names.bindProperties(binder(), Collections.singletonMap(Constants.PROPERTY_PRETTY_PRINT_PAYLOADS, "true"));
- bind(GsonModule.DateAdapter.class).to(GsonModule.Iso8601DateAdapter.class);
We'll probably need to bind something like this adapter to be able to deserialize json strings to Date objects. Let's see which format the target API uses.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#pullrequestreview-28884232
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Ignasi Barrera <no...@github.com>.
Great! I know it's quite difficult to get started with the jclouds code, so please, don't hesitate to ask anything you need. I'll be happy to help and provide all the details you need so you can understand how everything works. We are also in the #jclouds IRC channel at Freenode, so feel free to join there if you need real time feedback! :)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#issuecomment-288962056
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/6e5082f7) and [2.0.x](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/884327fa). Thanks!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#issuecomment-289035009
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Trevor Flanagan <no...@github.com>.
trevorflanagan requested changes on this pull request.
> +
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Module;
+import org.jclouds.apis.ApiMetadata;
+import org.jclouds.apis.BaseApiLiveTest;
+import org.jclouds.dimensiondata.cloudcontrol.DimensionDataCloudControlApi;
+import org.jclouds.dimensiondata.cloudcontrol.DimensionDataCloudControlApiMetadata;
+import org.jclouds.sshj.config.SshjSshClientModule;
+import org.testng.annotations.Test;
+
+import java.util.Properties;
+
+@Test(groups = "live")
+public class BaseDimensionDataCloudControlApiLiveTest extends BaseApiLiveTest<DimensionDataCloudControlApi> {
+
+ protected String ORG_ID = System
@jawnclarke can you remove the ORG_ID variable from here? It is not used.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#pullrequestreview-28683205
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Trevor Flanagan <no...@github.com>.
trevorflanagan commented on this pull request.
> + properties.setProperty(Constants.PROPERTY_CONNECTION_TIMEOUT, "1200000"); // 15 minutes
+ return properties;
+ }
+
+ public static class Builder extends BaseHttpApiMetadata.Builder<DimensionDataCloudControlApi, Builder> {
+
+ protected Builder() {
+ id("dimensiondata-cloudcontrol").name("DimensionData CloudControl API").identityName("user name")
+ .credentialName("user password")
+ .documentation(URI.create("http://www.dimensiondata.com/en-US/Solutions/Cloud"))
+ .defaultEndpoint("https://api-REGION.dimensiondata.com/").version("2.4")
+ .defaultProperties(DimensionDataCloudControlApiMetadata.defaultProperties())
+ .view(typeToken(ComputeServiceContext.class)).defaultModules(
+ ImmutableSet.<Class<? extends Module>>builder().add(DimensionDataCloudControlHttpApiModule.class)
+ .add(DimensionDataCloudControlParserModule.class)
+ .add(DimensionDataComputeServiceContextModule.class).build());
@nacx Before making too many changes I want to make sure I am doing the right thing. Your expectation on this commnet is that `DimensionDataComputeServiceContextModule` , `DimensionDataComputeServiceDependenciesModule` `DimensionDataCloudControlComputeServiceAdapter` are removed? As a result of removing those I also remove the unused transformation functions classes.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#discussion_r107867046
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by John Clarke <no...@github.com>.
@jawnclarke pushed 1 commit.
6702146 PR-375 Review Changes
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375/files/eab4b721f7b16899bae4c93ad661ee6bcd0bdfac..6702146f192adcfb5a0dece268fa6b3ada4d0fa1
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by John Clarke <no...@github.com>.
@jawnclarke pushed 1 commit.
eab4b72 Removing unused orgid from BaseDimensionDataCloudControlApiLiveTest.java
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375/files/180d889f8b855d211d7e2690e126426c8689eff8..eab4b721f7b16899bae4c93ad661ee6bcd0bdfac
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Ignasi Barrera <no...@github.com>.
Closed #375.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#event-1014360809
Re: [jclouds/jclouds-labs] [JCLOUDS-1255] Initial API structure and
get account API implementation (#375)
Posted by Trevor Flanagan <no...@github.com>.
@nacx thanks for taking the time to look at this. These comments are really helpful for us. @jawnclarke and I will work through the changes you requested.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/375#issuecomment-288960919