You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by tbouron <gi...@git.apache.org> on 2017/03/27 11:37:18 UTC
[GitHub] brooklyn-server pull request #613: Returns catalog items created via REST AP...
GitHub user tbouron opened a pull request:
https://github.com/apache/brooklyn-server/pull/613
Returns catalog items created via REST API when coming from an archive
There is still an issue when the `FEATURE_LOAD_BUNDLE_CATALOG_BOM` is enabled; the REST API won't return the added items as it is currently handled by the `CatalogBomScanner`
But, this feature is disabled by default so we can probably live with that for now
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tbouron/brooklyn-server fix/upload-archive
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/613.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #613
----
commit c17ac20b3473b99445523225d6b9a09f431425f4
Author: Thomas Bouron <th...@cloudsoftcorp.com>
Date: 2017-03-27T11:34:12Z
Returns catalog items created via REST API when coming from an archive
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #613: Returns catalog items created via REST AP...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/613#discussion_r108150799
--- Diff: rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java ---
@@ -261,15 +245,37 @@ public Response createFromArchive(byte[] zipInput) {
throw new IllegalArgumentException("Error installing catalog items", ex);
}
}
-
- return Response.status(Status.CREATED).build();
+
+ // TODO improve on this - If the FEATURE_LOAD_BUNDLE_CATALOG_BOM is enabled, the REST API won't return the
+ // added items which breaks the contract on the API endpoint.
+ return buildCreateResponse(catalogItems);
} catch (RuntimeException ex) {
throw WebResourceUtils.badRequest(ex);
} finally {
if (f!=null) f.delete();
if (f2!=null) f2.delete();
}
}
+
+ private Response buildCreateResponse(Iterable<? extends CatalogItem<?, ?>> catalogItems) {
+ log.info("REST created catalog items: "+catalogItems);
+
+ Map<String,Object> result = MutableMap.of();
+
+ for (CatalogItem<?,?> catalogItem: catalogItems) {
+ try {
+ result.put(catalogItem.getId(), CatalogTransformer.catalogItemSummary(brooklyn(), catalogItem, ui.getBaseUriBuilder()));
+ } catch (Throwable t) {
+ log.warn("Error loading catalog item '"+catalogItem+"' (rethrowing): "+t);
+ // unfortunately items are already added to the catalog and hard to remove,
+ // but at least let the user know;
+ // happens eg if a class refers to a missing class, like
+ // loading nosql items including mongo without the mongo bson class on the classpath
+ throw Exceptions.propagateAnnotated("At least one unusable item was added ("+catalogItem.getId()+")", t);
--- End diff --
I suggest you collect the exception in here rather than throwing it -
- let the loop run through all items, collect any failures that occur
- if there are any failures, loop through the catalog items that _were_ added and remove them again, before
- create a failure response with the details of each item that could not be added.
Having said that, I'm not sure there's much that really can go wrong above, as this is just creating the summary objects. It would be good to apply the logic above to the `CatalogItemLoader.scanForCatalog`, but I don't think that is within the scope of this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #613: Returns catalog items created via REST API when ...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/613
LGTM, will merge this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #613: Returns catalog items created via REST AP...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/613#discussion_r108149537
--- Diff: rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java ---
@@ -261,15 +245,37 @@ public Response createFromArchive(byte[] zipInput) {
throw new IllegalArgumentException("Error installing catalog items", ex);
}
}
-
- return Response.status(Status.CREATED).build();
+
+ // TODO improve on this - If the FEATURE_LOAD_BUNDLE_CATALOG_BOM is enabled, the REST API won't return the
+ // added items which breaks the contract on the API endpoint.
+ return buildCreateResponse(catalogItems);
} catch (RuntimeException ex) {
throw WebResourceUtils.badRequest(ex);
} finally {
if (f!=null) f.delete();
if (f2!=null) f2.delete();
}
}
+
+ private Response buildCreateResponse(Iterable<? extends CatalogItem<?, ?>> catalogItems) {
+ log.info("REST created catalog items: "+catalogItems);
+
+ Map<String,Object> result = MutableMap.of();
+
+ for (CatalogItem<?,?> catalogItem: catalogItems) {
+ try {
+ result.put(catalogItem.getId(), CatalogTransformer.catalogItemSummary(brooklyn(), catalogItem, ui.getBaseUriBuilder()));
--- End diff --
Can you break this line so it's readable on github?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #613: Returns catalog items created via REST AP...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/613
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #613: Returns catalog items created via REST AP...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/613#discussion_r108380227
--- Diff: rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java ---
@@ -146,33 +146,15 @@ public Response createFromYaml(String yaml) {
throw WebResourceUtils.forbidden("User '%s' is not authorized to add catalog item",
Entitlements.getEntitlementContext().user());
}
-
- Iterable<? extends CatalogItem<?, ?>> items;
+
try {
- items = brooklyn().getCatalog().addItems(yaml);
+ final Iterable<? extends CatalogItem<?, ?>> items = brooklyn().getCatalog().addItems(yaml);
+ return buildCreateResponse(items);
} catch (Exception e) {
e.printStackTrace();
--- End diff --
We should never use `e.printStackTrace()` (not introduced in this PR; I just spotted it while looking over these changes)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #613: Returns catalog items created via REST AP...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/613#discussion_r108149221
--- Diff: rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java ---
@@ -261,15 +245,37 @@ public Response createFromArchive(byte[] zipInput) {
throw new IllegalArgumentException("Error installing catalog items", ex);
}
}
-
- return Response.status(Status.CREATED).build();
+
+ // TODO improve on this - If the FEATURE_LOAD_BUNDLE_CATALOG_BOM is enabled, the REST API won't return the
--- End diff --
Could you expand this section a bit so it explains the issue in more detail
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #613: Returns catalog items created via REST AP...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/613#discussion_r108153810
--- Diff: rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java ---
@@ -261,15 +245,37 @@ public Response createFromArchive(byte[] zipInput) {
throw new IllegalArgumentException("Error installing catalog items", ex);
}
}
-
- return Response.status(Status.CREATED).build();
+
+ // TODO improve on this - If the FEATURE_LOAD_BUNDLE_CATALOG_BOM is enabled, the REST API won't return the
+ // added items which breaks the contract on the API endpoint.
+ return buildCreateResponse(catalogItems);
} catch (RuntimeException ex) {
throw WebResourceUtils.badRequest(ex);
} finally {
if (f!=null) f.delete();
if (f2!=null) f2.delete();
}
}
+
+ private Response buildCreateResponse(Iterable<? extends CatalogItem<?, ?>> catalogItems) {
+ log.info("REST created catalog items: "+catalogItems);
+
+ Map<String,Object> result = MutableMap.of();
+
+ for (CatalogItem<?,?> catalogItem: catalogItems) {
+ try {
+ result.put(catalogItem.getId(), CatalogTransformer.catalogItemSummary(brooklyn(), catalogItem, ui.getBaseUriBuilder()));
+ } catch (Throwable t) {
+ log.warn("Error loading catalog item '"+catalogItem+"' (rethrowing): "+t);
+ // unfortunately items are already added to the catalog and hard to remove,
+ // but at least let the user know;
+ // happens eg if a class refers to a missing class, like
+ // loading nosql items including mongo without the mongo bson class on the classpath
+ throw Exceptions.propagateAnnotated("At least one unusable item was added ("+catalogItem.getId()+")", t);
--- End diff --
@geomacy I move this code from the `createFromYaml` function, agree that it could be refactored but It is not in the scope of this PR. I would leave it for future improvement
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---