You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Вальо <no...@github.com> on 2016/04/13 01:28:34 UTC
[jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs-google/pull/171
-- Commit Summary --
* Support shortened hardwareId in GCE
-- File Changes --
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/config/GoogleComputeEngineServiceContextModule.java (8)
A google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/internal/GCETemplateBuilderImpl.java (59)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-google/pull/171.patch
https://github.com/jclouds/jclouds-labs-google/pull/171.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-google/pull/171
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Aled Sage <no...@github.com>.
@andreaturli for changing `MachineTypeToHardware`: I really like that idea! Looking in `GoogleComputeEngineServiceAdapter`, it uses `hardware.getUri()` so we are free to simplify the id, rather than having the id being a toString of the full uri. On a quick look, it seems we can safely change GCE's `MachineTypeToHardware.id` to be more like the other cloud providers.
@bostko do you want to try that?
---
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-google/pull/171#issuecomment-209306169
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Aled Sage <no...@github.com>.
@bostko thanks for this. There are three failing unit tests in `org.jclouds.googlecomputeengine.compute.GoogleComputeEngineServiceMockTest` - can you take a look at those please?
---
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-google/pull/171#issuecomment-209293295
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Aled Sage <no...@github.com>.
@bostko @andreaturli looking at this more, it would be better to inject the desired behaviour at the level of the `hardwareIdPredicate`.
Note the comment in `TemplateBuilderImpl.findHardwareWithId(Set)` that says "TODO: switch to GetHardwareStrategy in version 1.5"!
Can we change core jclouds to make `findHardwareWithId()` protected?
Or alternatively we could introduce something like a `GetHardwareStrategy` that is injected via guice? I'm not sure off-hand what the method signature would be for the `GetHardwareStrategy`, because it needs to know the configuration of the TemplateBuilder that influences the hardware choice. Would the `GetHardwareStrategy` just deal with ids, or would we also want it to do the work currently done in `buildHardwarePredicate()` that checks for cores, RAM, etc? And would it do the checks for image compatibility (see `supportsImagesPredicate`)?
---
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-google/pull/171#issuecomment-209302521
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Aled Sage <no...@github.com>.
> +
> + @Inject
> + protected GCETemplateBuilderImpl(@Memoized Supplier<Set<? extends Location>> locations, ImageCacheSupplier images,
> + @Memoized Supplier<Set<? extends Hardware>> hardwares, Supplier<Location> defaultLocation,
> + @Named("DEFAULT") Provider<TemplateOptions> optionsProvider, @Named("DEFAULT") Provider<TemplateBuilder> defaultTemplateProvider,
> + GetImageStrategy getImageStrategy, GoogleComputeEngineApi api) {
> + super(locations, images, hardwares, defaultLocation, optionsProvider, defaultTemplateProvider, getImageStrategy);
> + projectSelfLink = api.project().get().selfLink().toString();
> + }
> +
> + @Override
> + public TemplateBuilder hardwareId(String hardwareId) {
> + super.hardwareId(hardwareId);
> + if (!hardwareId.startsWith("https://") ) {
> + if (location == null) {
> + throw new IllegalStateException("GCE: first set locationId and then you can use shortened hardwareId");
Instead of failing in `hardwareId(String)` if `locationId(String)` has not yet been called, we could do it in build(). We could override `build()`: first check if hardwareId is set, and if so then check if we need to transform it; and then call `return super.build();`.
Thoughts?
---
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-google/pull/171/files/14e69c36fd91529e4108e6fa7feae9f6f54a587a#r59509064
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Aled Sage <no...@github.com>.
> + @Memoized Supplier<Set<? extends Hardware>> hardwares, Supplier<Location> defaultLocation,
> + @Named("DEFAULT") Provider<TemplateOptions> optionsProvider, @Named("DEFAULT") Provider<TemplateBuilder> defaultTemplateProvider,
> + GetImageStrategy getImageStrategy, GoogleComputeEngineApi api) {
> + super(locations, images, hardwares, defaultLocation, optionsProvider, defaultTemplateProvider, getImageStrategy);
> + projectSelfLink = api.project().get().selfLink().toString();
> + }
> +
> + @Override
> + public TemplateBuilder hardwareId(String hardwareId) {
> + super.hardwareId(hardwareId);
> + if (!hardwareId.startsWith("https://") ) {
> + if (location == null) {
> + throw new IllegalStateException("GCE: first set locationId and then you can use shortened hardwareId");
> + }
> +
> + this.hardwareId = projectSelfLink + "/zones/" + location.getId() + "/machineTypes/" + hardwareId;
Bikeshedding: preference to do this the other way round: first figure out what the corrected hardwareId should be, and then call `return super.hardwareId(hardwareId)`. The way you've written it, we first call super with potentially the wrong id, and then we duplicate the logic of the super method by setting the hardwareId field directly.
---
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-google/pull/171/files/14e69c36fd91529e4108e6fa7feae9f6f54a587a#r59508064
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Вальо <no...@github.com>.
> +
> + @Inject
> + protected GCETemplateBuilderImpl(@Memoized Supplier<Set<? extends Location>> locations, ImageCacheSupplier images,
> + @Memoized Supplier<Set<? extends Hardware>> hardwares, Supplier<Location> defaultLocation,
> + @Named("DEFAULT") Provider<TemplateOptions> optionsProvider, @Named("DEFAULT") Provider<TemplateBuilder> defaultTemplateProvider,
> + GetImageStrategy getImageStrategy, GoogleComputeEngineApi api) {
> + super(locations, images, hardwares, defaultLocation, optionsProvider, defaultTemplateProvider, getImageStrategy);
> + projectSelfLink = api.project().get().selfLink().toString();
> + }
> +
> + @Override
> + public TemplateBuilder hardwareId(String hardwareId) {
> + super.hardwareId(hardwareId);
> + if (!hardwareId.startsWith("https://") ) {
> + if (location == null) {
> + throw new IllegalStateException("GCE: first set locationId and then you can use shortened hardwareId");
@aledsage how we could do it in build()?
build is a private and a very long method. It has to divided into smaller parts and then it has to be made protected.
---
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-google/pull/171/files/14e69c36fd91529e4108e6fa7feae9f6f54a587a#r59513927
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Вальо <no...@github.com>.
`server.enqueue(jsonResponse("/project.json"));` doesn't seem to help for tests.
---
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-google/pull/171#issuecomment-209854321
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Вальо <no...@github.com>.
Closed #171.
--
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-google/pull/171#event-872956603
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Вальо <no...@github.com>.
@aledsage test failure is older and it is because of ProjectApiLiveTest.getProject() failing.
I will look after lunch how I could plug in `MachineTypeToHardware` to transform the hardwareId.
---
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-google/pull/171#issuecomment-209343864
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Ignasi Barrera <no...@github.com>.
Nice discussion and nice proposed change :)
Changing the id is a potential code breaker, so we can't apply this in 1.9.x. This said, and regardless of the preferred approach, we should make all the GCE IDs consistent. Currently nodes and images also have the `selfLink` as their IDs, so we should change them all (or change the TemplateBuilder accordingly so the same criteria applies to the `imageId` too if that makes sense).
---
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-google/pull/171#issuecomment-209308884
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Andrea Turli <no...@github.com>.
wouldn't be easier just to edit the id built by [MachineTypeToHardware](https://github.com/jclouds/jclouds-labs-google/blob/1.9.x/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/MachineTypeToHardware.java#L58) ?
jclouds will list the hardwareProfiles and will transform it using the above function. Then the standard templateBuilder will compare the hardwareId with the list of harwareProfiles available and will find a match. wdyt?
---
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-google/pull/171#issuecomment-209294837
Re: [jclouds/jclouds-labs-google] Support shortened hardwareId in GCE
(#171)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Inject
> + protected GCETemplateBuilderImpl(@Memoized Supplier<Set<? extends Location>> locations, ImageCacheSupplier images,
> + @Memoized Supplier<Set<? extends Hardware>> hardwares, Supplier<Location> defaultLocation,
> + @Named("DEFAULT") Provider<TemplateOptions> optionsProvider, @Named("DEFAULT") Provider<TemplateBuilder> defaultTemplateProvider,
> + GetImageStrategy getImageStrategy, GoogleComputeEngineApi api) {
> + super(locations, images, hardwares, defaultLocation, optionsProvider, defaultTemplateProvider, getImageStrategy);
> + projectSelfLink = api.project().get().selfLink().toString();
> + }
> +
> + @Override
> + public TemplateBuilder hardwareId(String hardwareId) {
> + super.hardwareId(hardwareId);
> + if (!hardwareId.startsWith("https://") ) {
> + if (location == null) {
> + throw new IllegalStateException("GCE: first set locationId and then you can use shortened hardwareId");
+1, the build method is the right place. Overriding the `resolveHardware` method would be a good option.
---
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-google/pull/171/files/14e69c36fd91529e4108e6fa7feae9f6f54a587a#r59511885