You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2018/08/28 08:09:53 UTC
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
GitHub user ahgittin opened a pull request:
https://github.com/apache/brooklyn-ui/pull/66
recognize memberSpec anywhere, and refresh blueprint within yaml editor
makes it easier for other entities to work with memberSpecs, in composer or yaml editor
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-ui minor-fixes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-ui/pull/66.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 #66
----
commit be28082d87333692ad03e299a645e5bfb9f427ac
Author: Alex Heneveld <al...@...>
Date: 2018-08-27T12:19:21Z
recognize memberSpec anywhere, and refresh blueprint within yaml editor
makes it easier for other entities to work with memberSpecs, in composer or yaml editor
----
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213254446
--- Diff: ui-modules/blueprint-composer/app/components/util/model/entity.model.js ---
@@ -19,9 +19,10 @@
import {Issue} from './issue.model';
import {Dsl, DslParser} from './dsl.model';
-const MEMBERSPEC_REGEX = /^(\w+\.)+[mM]ember[sS]pec$/;
-const FIRST_MEMBERSPEC_REGEX = /^(\w+\.)+first[mM]ember[sS]pec$/;
-const ANY_MEMBERSPEC_REGEX = /^(\w+\.)+(first)?[mM]ember[sS]pec$/;
+const MEMBERSPEC_REGEX = /^(\w+\.)*[mM]ember[sS]pec$/;
+const FIRST_MEMBERSPEC_REGEX = /^(\w+\.)*first[mM]ember[sS]pec$/;
+// TODO ideally we'd just look at type EntitySpec, not key name, but for now look at keyname, anything ending memberSpec
--- End diff --
we do have the model, maybe not perfect, but we use it already in some places -- eg in composer examine the declared type of a config key to figure out how to render it. there are some gaps to have this everywhere reliably but feels worth keeping the comment that that would be a better way.
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213274760
--- Diff: ui-modules/blueprint-composer/app/components/util/model/entity.model.js ---
@@ -115,6 +116,8 @@ export class Entity {
*
*/
touch() {
+ // include a summary to aid with debugging (otherwise log just shows the property lastUpdated)
+ this.summary = (this.type || "unset") + (this.id ? " "+this.id : "");
--- End diff --
By expanding the entity and clicking on each property, the console will show you the value. You can also get that by debugging the code directly rather than relying on console statements.
It feels a bit odd to duplicate existing data for the sake of debugging
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213224816
--- Diff: ui-modules/blueprint-composer/app/components/providers/blueprint-service.provider.js ---
@@ -302,13 +302,11 @@ function BlueprintService($log, $q, $sce, paletteApi, iconGenerator, dslService)
function refreshConfigMemberspecsMetadata(entity) {
let promiseArray = [];
- if (entity.isCluster()) {
--- End diff --
Is there really another case, outside a cluster or fabric where we can have a memberspec?
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213224067
--- Diff: ui-modules/blueprint-composer/app/views/main/yaml/yaml.state.js ---
@@ -46,6 +46,7 @@ function yamlStateController($scope, $rootScope, $timeout, blueprintService, brS
try {
blueprintService.setFromYaml(cm.getValue(), true);
+ blueprintService.refreshBlueprintMetadata();
--- End diff --
There is no point of refreshing the metadata here as the YAML state doesn't use this info. When you flip to the graphical view, this method will be called
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213274160
--- Diff: ui-modules/blueprint-composer/app/views/main/yaml/yaml.state.js ---
@@ -46,6 +46,7 @@ function yamlStateController($scope, $rootScope, $timeout, blueprintService, brS
try {
blueprintService.setFromYaml(cm.getValue(), true);
+ blueprintService.refreshBlueprintMetadata();
--- End diff --
Hum I see but I think it could be achieved differently. In any case, `blueprintService.refreshBlueprintMetadata()` does a couple of network request. However, this code is triggered on every keypress, that will probably degrade this user experience.
We probably want to revisit this by adding a debounce but I don't think it's a good idea to add this in the current form
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213278092
--- Diff: ui-modules/blueprint-composer/app/components/util/model/entity.model.js ---
@@ -115,6 +116,8 @@ export class Entity {
*
*/
touch() {
+ // include a summary to aid with debugging (otherwise log just shows the property lastUpdated)
+ this.summary = (this.type || "unset") + (this.id ? " "+this.id : "");
--- End diff --
okay i'll comment out and we can re-enable if it's wanted for debugging
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213275020
--- Diff: ui-modules/blueprint-composer/app/components/util/model/entity.model.js ---
@@ -19,9 +19,10 @@
import {Issue} from './issue.model';
import {Dsl, DslParser} from './dsl.model';
-const MEMBERSPEC_REGEX = /^(\w+\.)+[mM]ember[sS]pec$/;
-const FIRST_MEMBERSPEC_REGEX = /^(\w+\.)+first[mM]ember[sS]pec$/;
-const ANY_MEMBERSPEC_REGEX = /^(\w+\.)+(first)?[mM]ember[sS]pec$/;
+const MEMBERSPEC_REGEX = /^(\w+\.)*[mM]ember[sS]pec$/;
+const FIRST_MEMBERSPEC_REGEX = /^(\w+\.)*first[mM]ember[sS]pec$/;
+// TODO ideally we'd just look at type EntitySpec, not key name, but for now look at keyname, anything ending memberSpec
--- End diff --
True. Ideally, we would like to use only the type. My comment was more to explain why we do it rather than asking for a change
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213275122
--- Diff: ui-modules/blueprint-composer/app/components/providers/blueprint-service.provider.js ---
@@ -302,13 +302,11 @@ function BlueprintService($log, $q, $sce, paletteApi, iconGenerator, dslService)
function refreshConfigMemberspecsMetadata(entity) {
let promiseArray = [];
- if (entity.isCluster()) {
--- End diff --
Ah good point, this should be removed then.
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213249760
--- Diff: ui-modules/blueprint-composer/app/components/util/model/entity.model.js ---
@@ -115,6 +116,8 @@ export class Entity {
*
*/
touch() {
+ // include a summary to aid with debugging (otherwise log just shows the property lastUpdated)
+ this.summary = (this.type || "unset") + (this.id ? " "+this.id : "");
--- End diff --
without this when we `console.log(entity)` we can't identify different entities, and it looks weird, as it just shows `Entity { lastUpdated: xxxx }`.
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-ui/pull/66
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213277964
--- Diff: ui-modules/blueprint-composer/app/views/main/yaml/yaml.state.js ---
@@ -46,6 +46,7 @@ function yamlStateController($scope, $rootScope, $timeout, blueprintService, brS
try {
blueprintService.setFromYaml(cm.getValue(), true);
+ blueprintService.refreshBlueprintMetadata();
--- End diff --
have commented out and added comments. ideally we'd have a cache and then re-enable this but agree with you as it stands it is likely to be too chatty with this enabled.
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213224460
--- Diff: ui-modules/blueprint-composer/app/components/util/model/entity.model.js ---
@@ -19,9 +19,10 @@
import {Issue} from './issue.model';
import {Dsl, DslParser} from './dsl.model';
-const MEMBERSPEC_REGEX = /^(\w+\.)+[mM]ember[sS]pec$/;
-const FIRST_MEMBERSPEC_REGEX = /^(\w+\.)+first[mM]ember[sS]pec$/;
-const ANY_MEMBERSPEC_REGEX = /^(\w+\.)+(first)?[mM]ember[sS]pec$/;
+const MEMBERSPEC_REGEX = /^(\w+\.)*[mM]ember[sS]pec$/;
+const FIRST_MEMBERSPEC_REGEX = /^(\w+\.)*first[mM]ember[sS]pec$/;
+// TODO ideally we'd just look at type EntitySpec, not key name, but for now look at keyname, anything ending memberSpec
--- End diff --
We cannot as all we have is a YAML file, hence why we look for the key name
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213256773
--- Diff: ui-modules/blueprint-composer/app/components/providers/blueprint-service.provider.js ---
@@ -302,13 +302,11 @@ function BlueprintService($log, $q, $sce, paletteApi, iconGenerator, dslService)
function refreshConfigMemberspecsMetadata(entity) {
let promiseArray = [];
- if (entity.isCluster()) {
--- End diff --
yes -- `BrooklynNode` uses it as part of upgrade, test framework uses it i think, and there is a downstream project that wants to use this in a different type of cluster.
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213245289
--- Diff: ui-modules/blueprint-composer/app/views/main/yaml/yaml.state.js ---
@@ -46,6 +46,7 @@ function yamlStateController($scope, $rootScope, $timeout, blueprintService, brS
try {
blueprintService.setFromYaml(cm.getValue(), true);
+ blueprintService.refreshBlueprintMetadata();
--- End diff --
It could be useful for things in the YAML editor. Specifically there is a downstream project which wants this for action enablement. I could also see it wanted for code completion.
---
[GitHub] brooklyn-ui pull request #66: recognize memberSpec anywhere, and refresh blu...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/66#discussion_r213224315
--- Diff: ui-modules/blueprint-composer/app/components/util/model/entity.model.js ---
@@ -115,6 +116,8 @@ export class Entity {
*
*/
touch() {
+ // include a summary to aid with debugging (otherwise log just shows the property lastUpdated)
+ this.summary = (this.type || "unset") + (this.id ? " "+this.id : "");
--- End diff --
I don't get the idea behind the summary, we already the type and id within the object. Can you explain this please @ahgittin ?
---