Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
i think #646 adds what is missing for those tests to run better - TBC tho!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
been through it. looks good. may be some subtleties which will only come
out as we use it but feels like the right idea and rightly done. minor
changes. i'll look at the failing test
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
Still going through it, but so far looks really good.
The test
`CatalogYamlRebindTest.testRebindWithCatalogAndAppRebindCatalogItemIds` seems
to take a long time (28s). I'll look
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
I expect the code comments introduced in #573 will clear up
https://github.com/apache/brooklyn-server/pull/338#discussion_r94763625 .
---
If your project is set up for it, you can reply
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
@geomacy I think we want `catalogItemId` explicit as the catalog item ID of
the item, in addition to the "hierarchy" (which is maybe better called
`catalogItemsSearchPath` ?). It is a
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
Thanks @ahgittin I will have a look at #573 hopefully today or very soon.
Nice to have extra tests!
Regarding the persistence, basically in the persisted XML my change
replaces
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
As for `catalogItemId`, I think we should continue to populate and persist
this. It is useful to know from whence something came, and for audit purposes
this will become even more
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
Hi @geomacy - have added another commit to #573 ; hadn't realized other
inheritance semantics re catalog item id. It now has tests and several
explicit TODO comments which this PR should
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
Thanks @ahgittin and @neykov for comments; I will try to address the
remaining items as noted above
(https://github.com/apache/brooklyn-server/pull/338#issuecomment-281950519)
soon.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
Very cool. Haven't reviewed in depth but looks on point at a high level.
A few specific comments:
* this will conflict with
https://github.com/apache/brooklyn-server/pull/573 --
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
@neykov, have addressed some of the review comments, see the links in the
commit messages. Will add a couple more remarks above.
The main outstanding changes are those from the
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
Caught up and rebased against master. Going to make a start on the review
comments from Svet.
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/338
Caught up and rebased against master.
---
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
13 matches
Mail list logo
