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 2016/09/28 11:51:20 UTC
[GitHub] brooklyn-ui pull request #34: Use new pinned and constraints flag returned b...
GitHub user tbouron opened a pull request:
https://github.com/apache/brooklyn-ui/pull/34
Use new pinned and constraints flag returned by the REST API
This allows the UI to work out which parameters need to be displayed on the app wizard, following this convention:
- if a parameter if marked as `pinned`, it is always displayed
- if a parameter has a `required` constraint and empty `defaultValue`, it is displayed
Everything is sorted by the priority
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tbouron/brooklyn-ui fix/parameter-contraints
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-ui/pull/34.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 #34
----
commit b507049801f45b8a3fa353045ef6eac678761786
Author: Thomas Bouron <th...@cloudsoftcorp.com>
Date: 2016-09-28T11:41:54Z
Use new pinned and constraints flag to work out which parameter need to be displayed on the app wizard
----
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/34#discussion_r81371242
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,10 +680,12 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return !isNaN(cfg.priority) ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true || (cfg.contraints.indexOf('required') > -1 && typeof cfg.defaultValue === 'string' && cfg.defaultValue === '')) {
--- End diff --
Correct.
`defaultValue` on the other hand can be `null` and is serialized as `undefined`. The check above doesn't handle the `defaultValue is null` case.
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
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/34#discussion_r81367589
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,10 +680,12 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return !isNaN(cfg.priority) ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true || (cfg.contraints.indexOf('required') > -1 && typeof cfg.defaultValue === 'string' && cfg.defaultValue === '')) {
--- End diff --
> Would be weird to have a defaultValue the empty string and required at the same time, but you never know :)
That's the point of this check, what if as a blueprint author I want to ensure that one of my parameter has be filled without giving it a default?
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/34#discussion_r81365746
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,10 +680,12 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return !isNaN(cfg.priority) ? cfg.priority : 99999;
--- End diff --
Nitpick, The REST API is configured with `@JsonSerialize(include = JsonSerialize.Inclusion.NON_NULL)` so `null` values server-side won't be included in the response (will be `undefined`). Given that the original check `cfg.priority !== undefined` is more explicit - for example `isNaN(null) == false` so not really clear what's the purpose of the check when reading the code.
---
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-ui issue #34: Use new pinned and constraints flag returned by the R...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-ui/pull/34
@neykov Comments addressed
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/34#discussion_r81355465
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,8 +680,13 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return cfg.priority ? cfg.priority : 99999;
--- End diff --
`priority = 0` is a valid value
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-ui/pull/34
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
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/34#discussion_r81362080
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,8 +680,13 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return cfg.priority ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true) {
+ var html = _.template(RequiredConfigEntryHtml, {data:cfg});
+ that.$('.config-table').append(html);
+ } else if (cfg.constraints.indexOf('required') > - 1 && (!cfg.defaultValue || cfg.defaultValue === '')) {
--- End diff --
> '' is treated as false
Are you sure of that? I always check if a variable exists **and** different than empty because I had troubles in the past. I would update for:
```js
if (cfg.pinned === true || (typeof cfg.defaultValue === 'string' && cfg.defaultValue !== '')) {
var html = _.template(RequiredConfigEntryHtml, {data:cfg});
that.$('.config-table').append(html);
}
```
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/34#discussion_r81366730
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,10 +680,12 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return !isNaN(cfg.priority) ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true || (cfg.contraints.indexOf('required') > -1 && typeof cfg.defaultValue === 'string' && cfg.defaultValue === '')) {
--- End diff --
This doesn't handle `defaultValue == null` (or is null converted to `''` somethere?).
Could do:
```
cfg.pinned || (cfg.contraints.indexOf('required') > -1 && (cfg.defaultValue === undefined || cfg.defaultValue === ''))
```
`undefined` because `null` values won't be included in the response.
Would be weird to have a `defaultValue` the empty string and required at the same time, but you never 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] brooklyn-ui pull request #34: Use new pinned and constraints flag returned b...
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/34#discussion_r81372043
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,10 +680,12 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return !isNaN(cfg.priority) ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true || (cfg.contraints.indexOf('required') > -1 && typeof cfg.defaultValue === 'string' && cfg.defaultValue === '')) {
--- End diff --
Sorry but it's not possible. In case of `defaultValue == null` in java, [it is not serialised](https://github.com/apache/brooklyn-server/blob/master/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/ConfigSummary.java#L45-L46) therefore, the only possibilities are either `undefined` or a non empty string.
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/34#discussion_r81356873
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,8 +680,13 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return cfg.priority ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true) {
+ var html = _.template(RequiredConfigEntryHtml, {data:cfg});
+ that.$('.config-table').append(html);
+ } else if (cfg.constraints.indexOf('required') > - 1 && (!cfg.defaultValue || cfg.defaultValue === '')) {
--- End diff --
Both cases have the same body so can merge the check by `||`-ing the two alternatives (two make it clear that teartment is the same)
`''` is treated as false so could change `(!cfg.defaultValue || cfg.defaultValue === '')` to `!cfg.defaultValue`.
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/34#discussion_r81372539
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,10 +680,12 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return !isNaN(cfg.priority) ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true || (cfg.contraints.indexOf('required') > -1 && typeof cfg.defaultValue === 'string' && cfg.defaultValue === '')) {
--- End diff --
Agree, that's my point.
`defaultValue == null` on the server side is serialized as missing (`undefined`) on the wire.
Which means that `cfg.contraints.indexOf('required') > -1 && typeof cfg.defaultValue === 'string' && cfg.defaultValue === ''` will be `false` for `undefined`. So it will not be shown, even if required.
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
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/34#discussion_r81370861
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,10 +680,12 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return !isNaN(cfg.priority) ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true || (cfg.contraints.indexOf('required') > -1 && typeof cfg.defaultValue === 'string' && cfg.defaultValue === '')) {
--- End diff --
No it cannot. `contraints` [is always there](https://github.com/apache/brooklyn-server/blob/master/rest/rest-api/src/main/java/org/apache/brooklyn/rest/domain/EntityConfigSummary.java#L69) and empty if there isn't any
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/34#discussion_r81357381
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,8 +680,13 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return cfg.priority ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true) {
+ var html = _.template(RequiredConfigEntryHtml, {data:cfg});
+ that.$('.config-table').append(html);
+ } else if (cfg.constraints.indexOf('required') > - 1 && (!cfg.defaultValue || cfg.defaultValue === '')) {
--- End diff --
More importantly having a default value of 0 will mess the check - handle it explicitly (should not show if required & defaultVAlue = 0).
---
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-ui issue #34: Use new pinned and constraints flag returned by the R...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-ui/pull/34
Needs fixing the `priority=0` case, the rest is nitpicks.
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
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/34#discussion_r81368420
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,10 +680,12 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return !isNaN(cfg.priority) ? cfg.priority : 99999;
--- End diff --
Do you really want me to change it again?
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-ui/pull/34#discussion_r81370118
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,10 +680,12 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return !isNaN(cfg.priority) ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true || (cfg.contraints.indexOf('required') > -1 && typeof cfg.defaultValue === 'string' && cfg.defaultValue === '')) {
--- End diff --
That's the `defaultValue`, not `constraints`. It can (and will frequently) be null.
---
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-ui pull request #34: Use new pinned and constraints flag returned b...
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/34#discussion_r81367055
--- Diff: src/main/webapp/assets/js/view/application-add-wizard.js ---
@@ -680,10 +680,12 @@ define([
this.$('.required-config-loading').hide()
if (catalogEntryItem!=null && catalogEntryItem.config!=null) {
var that = this
- _.each(catalogEntryItem.config, function (cfg) {
- if (cfg.priority !== undefined) {
+ _.chain(catalogEntryItem.config).sortBy(function (cfg){
+ return !isNaN(cfg.priority) ? cfg.priority : 99999;
+ }).each(function (cfg) {
+ if (cfg.pinned === true || (cfg.contraints.indexOf('required') > -1 && typeof cfg.defaultValue === 'string' && cfg.defaultValue === '')) {
--- End diff --
> This doesn't handle defaultValue == null (or is null converted to '' somethere?).
Could do
This would never happens as the REST API returns the `Predicate.toString()`
---
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-ui issue #34: Use new pinned and constraints flag returned by the R...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-ui/pull/34
Thanks for the updates @tbouron, merging.
---
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.
---