Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/962#discussion_r186256657
--- Diff:
core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java
---
@@ -934,15 +937,17 @@ private void
collectCatalogItemsFromItemMetadataBlock(String sourceYaml, Managed
String sourcePlanYaml = planInterpreter.getPlanYaml();
if (resultLegacyFormat==null) {
- // horrible API but basically if `result` is null then add to
local unpersisted registry instead,
- // without forcing resolution and ignoring errors; this lets
us deal with forward references, but
- // we'll have to do a validation step subsequently. (already
we let bundles deal with persistence,
- // just need TODO to make sure we delete previously-persisted
things which now come through this path.)
- // NB: when everything is a bundle and we've removed all
scanning then this can be the _only_ path
- // and code can be massively simpler
- // TODO allow these to be set in catalog.bom ?
+ // horrible API but basically `resultLegacyFormat==null` means
use the new-style,
+ // adding from persisted bundles to type registry (which is
not persisted)
+ // instead of old way which persisted catalog items (and not
their bundles).
+ // this lets us deal with forward references, with a
subsequent step to validate.
+
+ Set<Object> tags =
MutableSet.of().putAll(getFirstAs(catalogMetadata, Collection.class,
"tags").orNull());
--- End diff --
This was a `List<Object>` previously, why the change to a `Set<Object>`? Is
that to remove duplicates? In this case, I don't think that is the right
behaviour, if the user wants multiple time the same tags, we should allow that.
Also, surely this will cause backward compatibilities issues
---
