Added Behavior classes for logic and get/setGroupConfiguration API call 2

reworked version of !526 (closed). Fixed bug with setting of applicationType, added default configurations to behaviors and extended error messages.

mentioned in merge request !526 (closed)

Build triggered.

Build Started: http://dsjenkins.cloudapp.net/jenkins/job/dSS_MR/655/

Build finished. Tests PASSED. Build results available at: http://dsjenkins.cloudapp.net/jenkins/job/dSS_MR/655/

assigned to @brano

added 1 commit

• 38415666 - Changes after review

Compare with previous version

Updated MR after comments. I was able to modify the JSONWriter to allow sending raw json objects and current response looks like this:

{"result":{"activeBasicScenes":[0,17,18]},"ok":true}

Build triggered.

Build Started: http://dsjenkins.cloudapp.net/jenkins/job/dSS_MR/662/

Build finished. Tests PASSED. Build results available at: http://dsjenkins.cloudapp.net/jenkins/job/dSS_MR/662/

I was able to modify the JSONWriter to allow sending raw json objects and current response looks like this: {"result":{"activeBasicScenes":[0,17,18]},"ok":true}

I would like to have confirmation from @mtross in either direction. Removing top-level configuration object is more concise but likely to be inconsistent with other methods. I don't have preferences in either direction. If @mtross stays silent, we merge what you push :)

@brano I don't get the question about "removing top-level configuration object", missing the context of commit here? The JSON response lgtm, or is something inconsistent?

@mtross We discussed these 2 options:

1. {"result":{"activeBasicScenes":[0,17,18]},"ok":true}
2. {"result":{"configuration":{"activeBasicScenes":[0,17,18]}},"ok":true}

All our json responses I am aware of (not really much and I did no special investigation) are consistent with option 2. Capnproto (also grpc) rpc encourages option 2 to allow to add another output argument in backwards compatible way. Thrift, avro rpc don't care. I don't have strong opinion, but I am slightly in favor of 2.

JSON response objects for other methods don't have this additional indirection. Usually the "result" object has the fields with the requested values. From the consistency point of view with other json methods I vote for the alternative without configuration.

I also do not have strong opinion on this so we can go with current state (option 1). One last questions are the location of API calls (zone or structure) and if we want to add applicationType to the call for clusters and user groups. See comment !526 (comment 36786).

added 1 commit

• 4418d7b1 - moved get/setGroupConfiguration from zone to structure

Compare with previous version

Build triggered.

Build Started: http://dsjenkins.cloudapp.net/jenkins/job/dSS_MR/671/

Just moved the handlers to structure. For me dealing with aplicationType is still not clear (required, optional, 0 etc.). I opt for requiring the argument for cluster and userApartmentApplications (userGA) and use the proper fixed one for all other groups. So dsm-api can stay as it is - return error if applicationType do not match and DSS will set valid arguments.

I opt for requiring the argument for cluster and userApartmentApplications (userGA) and use the proper fixed one for all other groups. So dsm-api can stay as it is - return error if applicationType do not match and DSS will set valid arguments.

+1 from me

resolved all discussions

Build finished. Tests PASSED. Build results available at: http://dsjenkins.cloudapp.net/jenkins/job/dSS_MR/671/

+1 review except the code in groupGetConfiguration, groupSetConfiguration.

But I would like to address them on master as it is new (still unused) code.

added 1 commit

• 9cb6f781 - Updates after review

Compare with previous version

Build triggered.

Build Started: http://dsjenkins.cloudapp.net/jenkins/job/dSS_MR/675/

Build finished. Tests FAILED. Build results available at: http://dsjenkins.cloudapp.net/jenkins/job/dSS_MR/675/

Only for groups with fixed applicationType?

Ok I will require it for clusters and user groups although the API becomes quite strict then. The checks if the change is possible are already implemented in manipulator.

What about adding something like Group::getFixedApplicationType() -> boost::optional<ApplicationType> / ApplicationType and use it here? To move more logic down into Group class.

I think that checking the group type in API call is enough for now not to complicate it further.

As you want. Just soft request towards better model / view separation.

mentioned in merge request !538 (merged)

Opened new MR after rebase on conflicting master (!538 (merged)). Closing this one to preserve comments.

closed