sort out ORIGINAL_SPEC and NAMED_SPEC_NAME, deprecating the latter and ensuring the former is always set to the originally requested spec, with tests
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/1e8873a8 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/1e8873a8 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/1e8873a8 Branch: refs/heads/master Commit: 1e8873a8d52a03267cbbbd06af1e70ba9a57fcf3 Parents: e2319e5 Author: Alex Heneveld <[email protected]> Authored: Tue Feb 28 16:01:05 2017 +0000 Committer: Alex Heneveld <[email protected]> Committed: Tue Feb 28 16:01:05 2017 +0000 ---------------------------------------------------------------------- .../camp/brooklyn/catalog/CatalogYamlLocationTest.java | 3 +++ .../brooklyn/core/location/BasicLocationRegistry.java | 12 ++++++++++-- .../brooklyn/core/location/CatalogLocationResolver.java | 3 +-- .../core/location/internal/LocationConfigMap.java | 1 - .../core/location/internal/LocationInternal.java | 6 +++--- .../brooklyn/rest/transform/LocationTransformer.java | 10 +++++----- 6 files changed, 22 insertions(+), 13 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/1e8873a8/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java ---------------------------------------------------------------------- diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java index 01fb484..026b2e8 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java @@ -34,6 +34,7 @@ import org.apache.brooklyn.api.typereg.RegisteredType; import org.apache.brooklyn.camp.brooklyn.AbstractYamlTest; import org.apache.brooklyn.core.config.BasicConfigKey; import org.apache.brooklyn.core.entity.Entities; +import org.apache.brooklyn.core.location.internal.LocationInternal; import org.apache.brooklyn.core.typereg.RegisteredTypePredicates; import org.apache.brooklyn.core.typereg.RegisteredTypes; import org.apache.brooklyn.entity.stock.BasicEntity; @@ -328,6 +329,8 @@ public class CatalogYamlLocationTest extends AbstractYamlTest { assertEquals(location.getConfig(new BasicConfigKey<String>(String.class, "config1")), "config1"); assertEquals(location.getConfig(new BasicConfigKey<String>(String.class, "config2")), "config2 override"); assertEquals(location.getConfig(new BasicConfigKey<String>(String.class, "config3")), "config3"); + + Asserts.assertEquals(location.getConfig(LocationInternal.ORIGINAL_SPEC), locTypeInYaml); } private void addCatalogLocation(String symbolicName, String locationType, List<String> libraries) { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/1e8873a8/core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java b/core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java index e68c0dc..a761513 100644 --- a/core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java +++ b/core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java @@ -347,7 +347,12 @@ public class BasicLocationRegistry implements LocationRegistry { .putAll(locationFlags) .putIfAbsentAndNotNull(LocationInternal.NAMED_SPEC_NAME, ld.getName()) .putIfAbsentAndNotNull(LocationInternal.ORIGINAL_SPEC, ld.getName()); - return getLocationSpec(ld.getSpec(), newLocationFlags.getAllConfigRaw()); + Maybe<LocationSpec<? extends Location>> result = getLocationSpec(ld.getSpec(), newLocationFlags.getAllConfigRaw()); + if (result.isPresent()) { + result.get().configure(LocationInternal.NAMED_SPEC_NAME, ld.getName()); + result.get().configure(LocationInternal.ORIGINAL_SPEC, ld.getName()); + } + return result; } @Override @@ -372,7 +377,10 @@ public class BasicLocationRegistry implements LocationRegistry { if (resolver != null) { try { - return (Maybe) Maybe.of(resolver.newLocationSpecFromString(spec, locationFlags, this)); + LocationSpec<? extends Location> specO = resolver.newLocationSpecFromString(spec, locationFlags, this); + specO.configure(LocationInternal.ORIGINAL_SPEC, spec); + specO.configure(LocationInternal.NAMED_SPEC_NAME, spec); + return (Maybe) Maybe.of(specO); } catch (RuntimeException e) { return Maybe.absent(Suppliers.ofInstance(e)); } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/1e8873a8/core/src/main/java/org/apache/brooklyn/core/location/CatalogLocationResolver.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/location/CatalogLocationResolver.java b/core/src/main/java/org/apache/brooklyn/core/location/CatalogLocationResolver.java index da66172..239fd16 100644 --- a/core/src/main/java/org/apache/brooklyn/core/location/CatalogLocationResolver.java +++ b/core/src/main/java/org/apache/brooklyn/core/location/CatalogLocationResolver.java @@ -67,8 +67,7 @@ public class CatalogLocationResolver implements LocationResolver { } LocationSpec<?> origLocSpec = managementContext.getTypeRegistry().createSpec(item, null, LocationSpec.class); - LocationSpec<?> locSpec = LocationSpec.create(origLocSpec).configure(locationFlags); - return locSpec; + return LocationSpec.create(origLocSpec).configure(locationFlags); } @Override http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/1e8873a8/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationConfigMap.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationConfigMap.java b/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationConfigMap.java index 689f5f2..51e2e61 100644 --- a/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationConfigMap.java +++ b/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationConfigMap.java @@ -44,7 +44,6 @@ import com.google.common.base.Predicate; public class LocationConfigMap extends AbstractConfigMapImpl<Location> { - @SuppressWarnings("unused") private static final Logger log = LoggerFactory.getLogger(LocationConfigMap.class); public LocationConfigMap(AbstractLocation loc) { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/1e8873a8/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationInternal.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationInternal.java b/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationInternal.java index 79b8eaa..9ed1767 100644 --- a/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationInternal.java @@ -37,10 +37,10 @@ import com.google.common.annotations.Beta; */ public interface LocationInternal extends BrooklynObjectInternal, Location { - // TODO original_spec seems to be the reference inside the named_spec -- do we really ever want that? public static final ConfigKey<String> ORIGINAL_SPEC = ConfigKeys.newStringConfigKey("spec.original", "The original spec used to instantiate a location"); - public static final ConfigKey<String> FINAL_SPEC = ConfigKeys.newStringConfigKey("spec.final", "The actual spec (in a chain) which instantiates a location"); - public static final ConfigKey<String> NAMED_SPEC_NAME = ConfigKeys.newStringConfigKey("spec.named.name", "The name on the (first) named spec in a chain"); + /** @deprecated since 0.11.0 use {@link #ORIGINAL_SPEC}; the distinction was unclear and the implementation inconsistent */ @Deprecated + public static final ConfigKey<String> NAMED_SPEC_NAME = ConfigKeys.newStringConfigKey("spec.named.name", "The name on the (first) named spec in a chain, either original spec or as defined in that"); + public static final ConfigKey<String> FINAL_SPEC = ConfigKeys.newStringConfigKey("spec.final", "The actual spec (in a chain) which instantiates a location, ie is not a reference to another one"); /** * Registers the given extension for the given type. If an extension already existed for http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/1e8873a8/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/LocationTransformer.java ---------------------------------------------------------------------- diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/LocationTransformer.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/LocationTransformer.java index c083ad9..fa3fb2c 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/LocationTransformer.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/LocationTransformer.java @@ -70,12 +70,13 @@ public class LocationTransformer { // mgmt not actually needed Map<String, Object> config = MutableMap.copyOf(explicitConfig==null ? null : explicitConfig.getAllConfig()); if (spec!=null && (level==LocationDetailLevel.FULL_EXCLUDING_SECRET || level==LocationDetailLevel.FULL_INCLUDING_SECRET)) { - // full takes from any resolved spec AND from explicit config - // TODO include flags? esp need provider, region, and endpoint + // full takes from any resolved spec ie inherited, AND explicit config and flags; + // would be nice to distinguish flags from config, but more important to make sure flags are supplied + // (ideally would return yaml, using yoml) config = ConfigBag.newInstance(spec.getFlags()).putAll(spec.getConfig()).putAll(config).getAllConfig(); } else if (level==LocationDetailLevel.LOCAL_EXCLUDING_SECRET) { - // in local mode, just make sure display name is set - // TODO include provider, region, and endpoint ? + // in local mode, make sure any inherited display name is set, but otherwise _no_ inherited + // NB this may not include provider, region, and endpoint -- use 'full' for that if (spec!=null && !explicitConfig.containsKey(LocationConfigKeys.DISPLAY_NAME) ) { if (Strings.isNonBlank((String) spec.getFlags().get(LocationConfigKeys.DISPLAY_NAME.getName()))){ config.put(LocationConfigKeys.DISPLAY_NAME.getName(), spec.getFlags().get(LocationConfigKeys.DISPLAY_NAME.getName())); @@ -148,7 +149,6 @@ public class LocationTransformer { // walk parent locations // TODO not sure this is the best strategy, or if it's needed, as the spec config is inherited anyway... if (spec==null) { - // TODO should be "named spec" ? Maybe<Object> originalSpec = ((LocationInternal)lp).config().getRaw(LocationInternal.ORIGINAL_SPEC); if (originalSpec.isPresent()) spec = Strings.toString(originalSpec.get());
