You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by sjcorbett <gi...@git.apache.org> on 2014/11/24 13:22:57 UTC
[GitHub] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
GitHub user sjcorbett opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/356
Adds boolean ConfigKey.isNonNull() method
Use to enforce non-null values for particular config keys.
Validity checks are made in `AbstractEntity.setConfigInternal`, so `setConfig(required, null)` should fail, and in `InternalEntityFactory` after setting config values on a newly-created entity, so forgetting to set a value on a required key should also fail.
The `InternalEntityFactory` check will only work for `ConfigKeys` defined on the interfaces implemented by the entity. I think the combination of the two checks is sufficient.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sjcorbett/incubator-brooklyn non-null-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/356.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 #356
----
commit 1eb10bc3e43b137105e2a4ef1b30447ddbafc68e
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date: 2014-11-24T12:19:21Z
Adds boolean ConfigKey.isNonNull() method
Use to enforce non-null values for particular config keys.
Validity checks are made in AbstractEntity.setConfigInternal, so
setConfig(required, null) should fail, and in InternalEntityFactory
after setting config values on a newly-created entity, so forgetting to
set a value on a required key should also fail.
----
---
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] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64686894
I'd argue it does simplify the code. It is not only preventing `set(null)`, it also provides a standard way to catch missing configuration when an entity is created. Currently, to check a ConfigKey was set you have to write something like:
```
@Override
public void init() {
checkNotNull(getConfig(MY_CONFIG), "Entity requires a value for MY_CONFIG");
}
```
This splits the statement of intent that a value for `MY_CONFIG` must be set when the entity is created away from definition of the key. There is no feasible way for us to generate documentation that indicates that `MY_CONFIG` must be set.
With the change in this pull request we can replace the check in the `init` method by defining the config key as:
```
ConfigKey<Object> MY_CONFIG = ConfigKeys.newConfigKey(Object.class)
.nonNull()
.build();
```
Attempts to create entities missing mandatory configuration are automatically caught in `InternalEntityFactory`. We can extend the jsgui to indicate required configuration when you're adding an app or an entity (and even prevent submissions that are missing such values) or are browsing the catalogue. We can also indicate required configuration in the catalogue documentation on the Brooklyn website.
The change is backwards compatible - all existing ConfigKeys remain nullable. The nulling-a-parents-key case is valid. I envisage this being used selectively rather than left, right and centre.
It should definitely be annotated `@Beta`.
---
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] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-72013306
@sjcorbett i still think it is too confusing having `nonNull()` verified at deployment time for entities. people routinely set config early in the start sequence but after deployment. if the `EntityFactory` check were remove i think we'd be fine, and we are clear that the meaning of `nonNull` is that a `get(key)` call never returns null.
i'm okay with merging it once the deploy-time check is removed. (that could be refactored to a utility method available on demand.)
however if you have bandwidth or there is not a pressing need for this now my preference would still be to apply a `ConfigValidation` scheme instead of this impl, and to do this now rather than introduce the api proposed here which i think we'll deprecate. a `ConfigValidation` scheme could have a `NON_NULL_ON_DEPLOY` or `NON_NULL_ON_USE` and we can cleanly do deploy-time checks. it could then be expanded with things like `MATCHES_REGEX` and `IN_RANGE` etc.
this could follow the `ConfigInheritance` pattern in https://github.com/apache/incubator-brooklyn/pull/483 .
(with apologies for the delay in replying, and for the merge conflicts that are now present)
---
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] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64499719
The code is good but there I'm not clear what problem this solves. We are adding an API method but it's not simplifying the code anywhere. It has the potential to do so I think but I'd like to see the use case elaborated.
The reason for this is that it feels like we're solving a slightly different problem to the one we want to. What we are currently doing is forbidding a user to set a `null`. I don't know of any places where we accidentally call `set(null)` so I don't think this is quite what we want to do.
I think what we want to do instead is to guarantee that a `get()` on a key will never return null. Currently however if there is no default value, we still might get null (if the key is never set). So I think `nonNull()` is useful mainly when used with `defaultValue(nonNullThing)`.
Also, in some places we set `null` to mean "don't inherit your parent's value". And that still might be useful. (But currently that only has the desired effect if the default value is null, or if the caller does `get(KEY)!=null ? get(KEY) : KEY.defaultValue()` ... and I'm not sure if that latter one is used in many places.)
So maybe what we want to do, instead of disallowing `set(null)`, would be to when we `get()` the value if the value is explicitly null and the key is `nonNull()`, then we return the default value. In other words, instead of `nonNull()` it could be called `useDefaultValueIfNull()` ? But I still wonder is that useful enough?
Are there use cases I'm not thinking of?
Also, if we do want to merge this, should we mark `nonNull()` and `isNonNull()` as `@Beta` ?
---
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] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64491793
@aledsage since you were working on config, what do you think?
---
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] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/356
---
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] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64779580
I think the intention is sound. Maybe call it `required` or `mandatory`?
---
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] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64787121
Aha so the goal is to make validation rules discoverable. I like that.
TOSCA/HOT/Heat have quite a rich notion of this - ranges, regexes etc.
Would it be better for the API to start down this route? eg. imagine a
.constraint(Constraints.NON_NULL)
Of course with it @Beta we can do this in the future instead. So that
shouldn't block this.
But I am concerned about the non-null check in the EntityFactory. I had
missed that. I don't like this for a few reasons:
- it limits our ability to set config post-deployment but pre-use
(originally that wasn't the intent of config but it is becoming more common
and I think that's good actually)
- it means non-null is enforced only on entities, not on policies or
anywhere else they are used (I'm thinking it would be nice to rename
ConfigKey and Bag to DataKey and Bag, to emphasise they could be used in
many places)
With that in mind would it make more sense to enforce constraints (ie the
non-null check, at this point) at the point of the get() call?
So if we wanted to validate a config at any point we just get() it. And if
we did want to enforce an "all config is set" in entity init, that could be
an easy one-line convenience that goes through the statics.
Actually thinking about this, where it could be really useful is if we had
a URL_EXISTS constraint - so many places we check those explicitly.
Best
Alex
On 26 Nov 2014 18:05, "Sam Corbett" <no...@github.com> wrote:
> I'd argue it does simplify the code. It is not only preventing set(null),
> it also provides a standard way to catch missing configuration when an
> entity is created. Currently, to check a ConfigKey was set you have to
> write something like:
>
> @Override
> public void init() {
> checkNotNull(getConfig(MY_CONFIG), "Entity requires a value for MY_CONFIG");
> }
>
> This splits the statement of intent that a value for MY_CONFIG must be
> set when the entity is created away from definition of the key. There is no
> feasible way for us to generate documentation that indicates that
> MY_CONFIG must be set.
>
> With the change in this pull request we can replace the check in the init
> method by defining the config key as:
>
> ConfigKey<Object> MY_CONFIG = ConfigKeys.newConfigKey(Object.class)
> .nonNull()
> .build();
>
> Attempts to create entities missing mandatory configuration are
> automatically caught in InternalEntityFactory. We can extend the jsgui to
> indicate required configuration when you're adding an app or an entity (and
> even prevent submissions that are missing such values) or are browsing the
> catalogue. We can also indicate required configuration in the catalogue
> documentation on the Brooklyn website.
>
> The change is backwards compatible - all existing ConfigKeys remain
> nullable. The nulling-a-parents-key case is valid. I envisage this being
> used selectively rather than left, right and centre.
>
> It should definitely be annotated @Beta.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64686894>
> .
>
---
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] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-72014751
Thanks for your thoughts. I got as far as a draft to the dev list previously. I'm keen to keep the deploy-time check in some form - it's annoying to wait 15 minutes for your cloud to provision an instance and only to then fail because you forgot to set some config that's required by `customize`. I'll close this and work towards the `ConfigValidation` idea.
---
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] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64884664
I really like the idea of constraints (or preconditions?). Shall we continue this discussion on the mailing list?
Does anybody want me to make further changes to this changeset?
---
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] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64491730
Another thing that might be useful is for `ConfigKey#getOptionalValue` to return an `Optional<X>` to allow clients to determine whether a key was set, rather than returning the default. Perhaps a range of methods along the lines of `getConfigRaw` but with more (i.e. _all possible_) options for inheritance and defaults.
---
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.
---