Adds constraint (required|forbidden)unlessAnyOf

[aledsage]

No description provided.

[kemitix]

The tests aren't 'readable'. The calls to assertKeyBehaviour() method seem somewhat cryptic. As a niave reader of the tests, I've no idea what is being checked unless I also know the internals of the method being called.

[aledsage]

retest this please

Changed test config, to enable Delete workspace before build starts

[aledsage]

@kemitix I completely agree about the tests being cryptic. Passing four booleans in a row in a method signature is an anti-pattern: it is hard to read and very error-prone.

These tests were originally written by Alex - I did some minor improvements to their readability (see d19d207). I decided it was hard to make it much more readable without significantly increasing the amount of code required.

On balance, because it is a test class and because the methods are private, it seemed ok to leave the methods as cryptic - unpleasant for code reviewers looking at a diff, but if you open up the class it's liveable with.

[geomacy]

LGTM.

I tested this with a catalog

brooklyn.catalog:
  bundle: my-test
  version: 0.0.1-SNAPSHOT
  id: my-test
  name: TTest

  items:

  - id: TypeA
    itemType: entity
    item:
      type: org.apache.brooklyn.entity.stock.BasicEntity
      brooklyn.parameters:
      - name: red
        type: string
      - name: green
        type: string
      - name: blue
        type: string
        constraints:
        - requiredUnlessAnyOf:
          - red
          - green
        
  - id: TypeB
    itemType: entity 
    item:
      type: org.apache.brooklyn.entity.stock.BasicEntity
      brooklyn.parameters:
      - name: red
        type: string
      - name: green
        type: string
      - name: blue
        type: string
        constraints:
        - forbiddenUnlessAnyOf:
          - red
          - green

and deployed assorted test applications as below with the results indicated:

name: test1
location: localhost
services:
- type: TypeA

Error deploying:
Application deployment failed
Error launching blueprint: Error configuring Basic Entity: [blue:requiredUnlessAnyOf("red", "green")]: ConstraintViolationException: Error configuring Basic Entity: [blue:requiredUnlessAnyOf("red", "green")]
= SUCCESS

name: test2
location: localhost
services:
- type: TypeA
  brooklyn.config:
    red: yes

Deployed OK = SUCCESS

name: test2b
location: localhost
services:
- type: TypeA
  brooklyn.config:
    green: yes

Deployed OK = SUCCESS

name: test3
location: localhost
services:
- type: TypeA
  brooklyn.config:
    blue: yes

Deployed OK = SUCCESS!

name: test4
location: localhost
services:
- type: TypeB

Deployed OK = SUCCESS!

name: test5
location: localhost
services:
- type: TypeB
  brooklyn.config:
    blue: yes

Error deploying:
Application deployment failed
Error launching blueprint: Invalid value for blue[ConfigKey:java.lang.String] on BasicEntityImpl{id=frgzvs3cd4} (yes); it should satisfy forbiddenUnlessAnyOf("red", "green"): ConstraintViolationException: Invalid value for blue[ConfigKey:java.lang.String] on BasicEntityImpl{id=frgzvs3cd4} (yes); it should satisfy forbiddenUnlessAnyOf("red", "green")
= SUCCESS

name: test6
location: localhost
services:
- type: TypeB
  brooklyn.config:
    red: yes
    blue: yes

Deployed OK = success

name: test6b
location: localhost
services:
- type: TypeB
  brooklyn.config:
    green: yes
    blue: yes

Deployed OK = SUCCESS

name: test7
location: localhost
services:
- type: TypeB
  brooklyn.config:
    someOtherArbitraryKey: yes
    blue: yes

Deployment failed:
Application deployment failed
Error launching blueprint: Invalid value for blue[ConfigKey:java.lang.String] on BasicEntityImpl{id=pr5xdyhrzw} (yes); it should satisfy forbiddenUnlessAnyOf("red", "green"): ConstraintViolationException: Invalid value for blue[ConfigKey:java.lang.String] on BasicEntityImpl{id=pr5xdyhrzw} (yes); it should satisfy forbiddenUnlessAnyOf("red", "green")
= SUCCESS

I'd say it's worth checking the above but I think the new constraints are working as advertised.

I agree with @kemitix that the tests are a bit cryptic, but on the other hand lots of the tests in Brooklyn are pretty cryptic so I wouldn't let that hold this PR up. I would merge this PR myself, but, @aledsage, it might at least be worth the addition of a TODO comment to recommend making these a little clearer to read. I leave it up to you, I'm happy for this to be merged either way.

[geomacy]

Oh by the way I had to jump through a hoop or two to get Brooklyn built with this - I was initially getting build errors from brooklyn-ui with this in place; I had to rebase this PR against master, and even at that got a build error as follows when building from the brooklyn parent:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 02:07 min
[INFO] Finished at: 2018-10-21T22:09:15+01:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.karaf.tooling:karaf-maven-plugin:4.1.6:verify (verify-brooklyn-ui-modularity-feature) on project brooklyn-ui-modularity-features: Verification failures: Verification failures:
[ERROR] 	Unable to resolve framework features
[ERROR] 	Unable to resolve framework features
[ERROR] 	Unable to resolve framework features
[ERROR] -> [Help 1]

oddly when I did

cd brooklyn-ui/modularity-server/features
mvn clean install -DskipTests

it worked, and then when I went back to the top and resumed the original build, that finally passed. What's that all about?

[geomacy]

Will merge this