You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2015/03/17 17:10:38 UTC
[GitHub] incubator-brooklyn pull request: BROOKLYN-136: persist dynamic loc...
GitHub user aledsage opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/555
BROOKLYN-136: persist dynamic locations
See javadoc on `BasicLocationRegistry` for a description of how this all hangs together.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn feature/BROOKLYN-136-persist-dynamicLocations
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/555.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 #555
----
commit d38788771c5ed67d7ab8fbdf105995a252174932
Author: Aled Sage <al...@gmail.com>
Date: 2015-03-17T14:46:28Z
Adds Yamls.getAt utility
commit 3f52957d21421357003f70f847778167e6652e1b
Author: Aled Sage <al...@gmail.com>
Date: 2015-03-17T14:46:57Z
Persist dynamically added locations in catalog
- Support CatalogItem of type location
- Location items added to catalog automatically added to
LocationRegistry
- LocationResource.create adds new location type to Catalog
----
---
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] incubator-brooklyn pull request: BROOKLYN-136: persist dynamic loc...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/555#issuecomment-82818563
A couple of minor comments, but other than that, LGTM. EmptySoftwareProcessYamlTest.testWithAppAndEntityLocations passes on my machine
---
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] incubator-brooklyn pull request: BROOKLYN-136: persist dynamic loc...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/555#issuecomment-82901499
Thanks @ahgittin @nakomis
For the failing `EmptySoftwareProcessYamlTest.testWithAppAndEntityLocations`, other tests were previously failing non-deterministically for me because the `start()` effector was kicked off asynchronously. For this test, I presume nothing was checking if the effector had actually executed successfully. Now that the code blocks for `start()` to complete, we get this failure. Have marked those as integration.
Have incorporated review comments, and rebased. Will wait for jenkins, and then merge.
---
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] incubator-brooklyn pull request: BROOKLYN-136: persist dynamic loc...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/555#discussion_r26607509
--- Diff: utils/common/src/main/java/brooklyn/util/yaml/Yamls.java ---
@@ -66,6 +66,47 @@
return (T)x;
}
+ /**
+ * Parses the given yaml, and walks the given path to return the referenced object.
+ *
+ * @see #getAt(Object, List)
+ */
+ public static Object getAt(String yaml, List<String> path) {
+ Iterable<Object> result = new org.yaml.snakeyaml.Yaml().loadAll(yaml);
+ Object current = result.iterator().next();
+ return getAtPreParsed(current, path);
+ }
+
+ /**
+ * For pre-parsed yaml, walks the maps/lists to return the given sub-item.
+ * In the given path:
+ * <ul>
+ * <li>A vanilla string is assumed to be a key into a map.
+ * <li>A string in the form like "[0]" is assumed to be an index into a list
+ * </ul>
+ *
+ * Returns {@code null} if that path does not exist.
+ */
+ @SuppressWarnings("unchecked")
+ public static Object getAtPreParsed(Object current, List<String> path) {
--- End diff --
ugly method name. if it has already been parsed is there any difference between this and `Jsonya.of(current).at(path).get()` ?
(at least mark `@Beta`)
---
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] incubator-brooklyn pull request: BROOKLYN-136: persist dynamic loc...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/555#issuecomment-82556967
very useful, and looks sensible. would be good to have a bigger review but as this is new functionality i'm not opposed to merging it and reviewing as we go along. good tests.
the other thing which occurs to me is that maybe we could get rid of some of the redundancy between definitions, specs, etc, by switching to catalog items. not sure about this but something to think about.
---
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] incubator-brooklyn pull request: BROOKLYN-136: persist dynamic loc...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/555#issuecomment-82554374
failure is `io.brooklyn.camp.brooklyn.EmptySoftwareProcessYamlTest.testWithAppAndEntityLocations`
```
java.util.concurrent.ExecutionException: brooklyn.util.exceptions.PropagatedRuntimeException: Error invoking start at BasicApplicationImpl{id=gl0S5Wg7}: Error invoking start at BasicApplicationImpl{id=gl0S5Wg7}: Error invoking start at EmptySoftwareProcessImpl{id=AVEPF1IO}: Cannot establish ssh connection to jenkins @ SshMachineLocation[SshMachineLocation:tUEe:127.0.0.1/127.0.0.1:22@tUEehcA5] (Exhausted available authentication methods).
```
am guessing this should be an integration test although why it's appearing now i don't know
---
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] incubator-brooklyn pull request: BROOKLYN-136: persist dynamic loc...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/555
---
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.
---