-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BROOKLYN-248: fix location displayName inheritance #104
Conversation
DO NOT MERGE: although this fixes it for the location test case, it breaks the expected behaviour for locations (see failing jenkins test). |
fd64e22
to
79fe765
Compare
STILL DO NOT MERGE. I've added more test cases, and reverted the change to |
79fe765
to
4e2114d
Compare
Good to review and merge now (assuming jenkins doesn't find any problems). |
I pulled and built a few minutes ago and tried with this catalog:
And I still see both locations I also tried adding On a slight tangent, building up locations from other locations lends itself to adding base locations that aren't useable by themselves, for example a base that only sets common private keys - currently those building-block locs show up in the list of locations via the jsgui - it'd be nice to have some way of flagging whether a location gets presented to a user in the UI - either a param or naming convention. Worth considering as a future enhancement? |
LGTM, merging. |
oops, just saw @johnmccabe's comment. Can you retest as this was updated minutes ago as well. Tests suggest your case should be working. |
@johnmccabe your example works for me when I build and run brooklyn locally. I don't have any files not committed or anything like that. |
@aledsage I've deleted my PR branch and am rebuilding again (I'd rebuilt earlier to sanity check my observations... third time lucky) |
@aledsage looks like something messed up my side... it works as expected now. Previously I was deleting then pulling the pr into a branch on brooklyn-server, doing a This time I deleting the pr branch and pulled it again, and instead built from the uber brooklyn project. Sorry for the false alarm. |
LGTM 👍 |
thanks @johnmccabe, merging. |
No description provided.