Improve config key descriptions #819

aledsage · 2017-09-13T09:07:14Z

See individual commits for details.

Note the last two commits make significant changes to config:

remove the default for launch.command of VanillaSoftwareProcess.
Rename some config keys (deprecating the old names) for latch.* and skip.*.

Also marks some most important config keys as ‘CatalogConfig’. And fixes some deprecation warnings in ConfigKey declarations.

Previously value was `./start.sh` which was confusing if someone saw it in an error message, and was pretty much never right.

More consistent naming will help, especially in UIs where config is listed in alphabetical order - the “latch.*” config will all be together. (Deprecates old values).

aledsage · 2017-09-13T10:14:04Z

retest this please

tbouron

Very minor comments but LGTM, thanks @aledsage !

On a side note, I'm wondering if we should also take care of icons for the various entities/enrichers/policies. I appreciate that it's not in the scope of this PR but worth thinking about it since we are improving catalog items

tbouron · 2017-09-13T10:42:45Z

core/src/main/java/org/apache/brooklyn/core/entity/Attributes.java

-            String.class, "download.url", "URL pattern for downloading the installer (will substitute things like ${version} automatically)");
+            String.class, 
+            "download.url", 
+            "URL pattern for downloading the installer (will substitute things like ${version} automatically)");


Shouldn't it just say uses FreeMarker templating format to be consistent with the other description below?

tbouron · 2017-09-13T10:42:52Z

core/src/main/java/org/apache/brooklyn/core/entity/Attributes.java

-            Map.class, "download.addon.urls", "URL patterns for downloading named add-ons (will substitute things like ${version} automatically)");
+            Map.class, 
+            "download.addon.urls", 
+            "URL patterns for downloading named add-ons (will substitute things like ${version} automatically)");


Shouldn't it just say uses FreeMarker templating format to be consistent with the other description below?

tbouron · 2017-09-13T10:44:15Z

core/src/main/java/org/apache/brooklyn/core/entity/BrooklynConfigKeys.java


    public static final ConfigKey<Duration> START_TIMEOUT = newConfigKey(
-            "start.timeout", "Time to wait for process and for SERVICE_UP before failing (in seconds, default 2m)", Duration.seconds(120));
+            "start.timeout", 
+            "Time to wait, after launching, for SERVICE_UP before failing (default to '2m')", 


Not sure the default needs to be written in the description. Would be hard to maintain and the CLI / UI have access to this info anyway.

tbouron · 2017-09-13T10:49:03Z

core/src/main/java/org/apache/brooklyn/entity/group/DynamicFabric.java

-            new TypeToken<EntitySpec<?>>() {}, "dynamiccfabric.memberspec", "entity spec for creating new cluster members", null);
+            new TypeToken<EntitySpec<?>>() {}, 
+            "dynamiccfabric.memberspec", 
+            "entity spec for creating new members (one per location)", 


s/entity/Entity/

tbouron · 2017-09-13T10:49:10Z

core/src/main/java/org/apache/brooklyn/entity/group/DynamicFabric.java

-            new TypeToken<EntitySpec<?>>() {}, "dynamiccfabric.firstmemberspec", "entity spec for creating new cluster members", null);
+            new TypeToken<EntitySpec<?>>() {}, 
+            "dynamiccfabric.firstmemberspec", 
+            "entity spec for the first member", 


s/entity/Entity/

aledsage · 2017-09-13T12:34:29Z

Thanks @tbouron - comments addressed.

@drigodwin I believe you were also looking at this? Ready to merge?

drigodwin

Looks good, thanks @aledsage

drigodwin · 2017-09-13T12:39:08Z

core/src/main/java/org/apache/brooklyn/enricher/stock/Aggregator.java

                    "'all', 'allAndAtLeastOne', 'atLeastOne', 'atLeastOneUnlessEmpty', 'alwaysHealthy'", "allAndAtLeastOne");

    public static final ConfigKey<Integer> QUORUM_TOTAL_SIZE = ConfigKeys.newIntegerConfigKey(
-            "quorum.total.size", "The total size to consider when determining if quorate", 1);
+            "quorum.total.size", 
+            "The total size to consider when determining if quorate (used iwth transformation of type 'isQuorate')", 1);


typo iwth -> with

ahgittin · 2017-09-14T07:42:20Z

core/src/main/java/org/apache/brooklyn/core/entity/BrooklynConfigKeys.java

+    public static final ConfigKey<Boolean> SKIP_ENTITY_START_IF_RUNNING = ConfigKeys.builder(Boolean.class)
+            .name("skip.start.ifRunning") 
+            .deprecatedNames("entity.running") 
+            .description("Whether to skip the startup process entirely, but only if it already running")


Whether to skip the startup process if the entity is detected as already running ?

ahgittin · 2017-09-14T07:43:44Z

core/src/main/java/org/apache/brooklyn/enricher/stock/Joiner.java

    public static final ConfigKey<Sensor<?>> TARGET_SENSOR = ConfigKeys.newConfigKey(new TypeToken<Sensor<?>>() {},
-            "enricher.targetSensor");
+            "enricher.targetSensor",
+            "The sensor that will be set on the associated entity, with the target value");


The sensor to be set on the associated entity with the value computed here ?

ahgittin · 2017-09-14T07:45:56Z

a couple minor suggestions of wording

LGTM

as per @tbouron - icons v important (more important than config description IMHO!)

tbouron · 2017-09-14T08:09:12Z

@aledsage I just realised that you miss one more bom file in karaf/init/src/main/resources/catalog-classes.bom. This one is the base for the karaf distribution.

(I think we should reuse the same base bom files for the classic and Karaf distributions, but not in the scope of this PR)

aledsage · 2017-09-15T11:18:14Z

@ahgittin @tbouron @drigodwin thanks for comments - incorporated in an additional commit. Is it ok to merge (with icons being separate from this PR)?

tbouron · 2017-09-15T11:36:52Z

@aledsage Ok to me 👍 (build failure looks unrelated)

drigodwin

Thanks @aledsage

aledsage added 5 commits September 12, 2017 18:31

Improve config key descriptions

3d62a1e

Also marks some most important config keys as ‘CatalogConfig’. And fixes some deprecation warnings in ConfigKey declarations.

catalog.bom: improve names/descriptions

651f63c

Deprecate AbstractGroup.MEMBER_DELEGATE_NAME_FORMAT

cb416a2

VanillaSoftwareProcess: delete default ‘launch.command’ val

8fa7b78

Previously value was `./start.sh` which was confusing if someone saw it in an error message, and was pretty much never right.

ConfigKeys: rename “latch.*” and “skip.*”

9ae4944

More consistent naming will help, especially in UIs where config is listed in alphabetical order - the “latch.*” config will all be together. (Deprecates old values).

tbouron reviewed Sep 13, 2017

View reviewed changes

PR apache#819 config descriptions: incorporate comments

d465091

drigodwin approved these changes Sep 13, 2017

View reviewed changes

ahgittin reviewed Sep 14, 2017

View reviewed changes

PR apache#819 config descriptions: incorporate comments (2)

98297ce

drigodwin approved these changes Sep 15, 2017

View reviewed changes

asfgit merged commit 98297ce into apache:master Sep 15, 2017

asfgit pushed a commit that referenced this pull request Sep 15, 2017

This closes #819

001730b

aledsage deleted the config-key-descriptions branch September 15, 2017 16:20

drigodwin mentioned this pull request Oct 3, 2017

Update config keys' deprecated names brooklyncentral/brooklyn-ceph#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve config key descriptions #819

Improve config key descriptions #819

aledsage commented Sep 13, 2017

aledsage commented Sep 13, 2017

tbouron left a comment

tbouron Sep 13, 2017

tbouron Sep 13, 2017

tbouron Sep 13, 2017

tbouron Sep 13, 2017

tbouron Sep 13, 2017

aledsage commented Sep 13, 2017

drigodwin left a comment

drigodwin Sep 13, 2017

ahgittin Sep 14, 2017

ahgittin Sep 14, 2017

ahgittin commented Sep 14, 2017

tbouron commented Sep 14, 2017

aledsage commented Sep 15, 2017

tbouron commented Sep 15, 2017

drigodwin left a comment

Improve config key descriptions #819

Improve config key descriptions #819

Conversation

aledsage commented Sep 13, 2017

aledsage commented Sep 13, 2017

tbouron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledsage commented Sep 13, 2017

drigodwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahgittin commented Sep 14, 2017

tbouron commented Sep 14, 2017

aledsage commented Sep 15, 2017

tbouron commented Sep 15, 2017

drigodwin left a comment

Choose a reason for hiding this comment