CP-26098: Port v6d interface from Camlp4 to PPX #186

minishrink · 2018-01-08T13:13:50Z

This PR is the second of three which port the licensing daemon to PPX:

xcp-idl: port V6d interface to use PPX
xapi: update references to v6 interface
v6d: port daemons to ppx.

This PR in particular includes the following changes:

ports v6d_interface from Camlp4 to PPX
updates the v6 jbuild file to no longer use camlp4
updates v6_client error handling

Possible follow-up: A .mli file defining the v6 interface
Note: CP-26098 is part of REQ-417

coveralls · 2018-01-08T13:29:44Z

Coverage remained the same at 54.268% when pulling 068682d on minishrink:ppx into 144fa27 on xapi-project:master.

lindig

This looks good. Could you group this into fewer commits? I have minor comments.

lindig · 2018-01-08T13:25:03Z

v6/v6_client.ml


+let json_url () = "file:" ^ json_path


I believe a file URL needs to start with file://, see https://en.wikipedia.org/wiki/File_URI_scheme

Happy to change it, but I'm curious: I've had a look at how other daemon interfaces define the json_url/json_path function, and it seems like they all follow this pattern, which would presumably result in something like this:
file:/var/path/to/file.extension
Given that there haven't been any issues so far (that I know of) with this format, is the change just a matter of following conventions? Also, is it worth changing for all interfaces?

I might have raised this in the past. It's wrong but it works. So let's keep it.

I can leave this for now and change it for all interfaces in a separate PR, would that work?

Sure. Let's stick with it for the moment

lindig · 2018-01-08T13:28:00Z

v6/v6_interface.ml

+(** handle exception generation and raising *)
+let err = Error.{
+    def = errors;
+    raiser = (function


Either both, raiser and matcher, should be in parenthesis, or none.

minishrink · 2018-01-08T13:39:01Z

Is there a way to group this into smaller commits without force pushing?

lindig · 2018-01-08T13:52:41Z

No. I would suggest you do this after all comments have been addressed but before we merge.

coveralls · 2018-01-08T14:27:25Z

Coverage remained the same at 54.268% when pulling 2763830 on minishrink:ppx into 144fa27 on xapi-project:master.

minishrink · 2018-01-08T15:41:56Z

If this PR is ready, I can merge this branch into it.

mseri

Just a minor thing. It looks good overall

mseri · 2018-01-09T14:03:59Z

v6/jbuild

  (flags (:standard -w -39 %s))
+  (modules (:standard))


This is the default value, you can delete this line

mseri · 2018-01-09T14:06:18Z

v6/v6_interface.ml

+[@@deriving rpcty]
+
+(** handle exception generation and raising *)
+let err = Error.{


A PR to ocaml-rpc to autogenerate err passing the exception wrapper type into a functor is welcome :)

coveralls · 2018-01-10T11:50:49Z

Coverage remained the same at 54.268% when pulling b917654 on minishrink:ppx into 144fa27 on xapi-project:master.

lindig · 2018-01-11T14:48:15Z

Is this ready to go in, @mseri?

robhoes · 2018-01-15T11:35:48Z

Quick observation: the commit messages don't have "Signed-off-by" lines yet.

robhoes

This like quite good, I've just added some minor comments.

However, the way this PR is split into commits does not make a lot of sense to me. Each commit should represent a logical change that is useful (and reviewable) on its own. But this looks more like a sequence of changes and corrections, and whitespace fixes mixed together. I'd personally just squash all changes to v6_interface.ml into a single patch. And separate ones for the client and jbuild file perhaps.

robhoes · 2018-01-15T13:39:24Z

v6/v6_interface.ml

-exception License_expired
-exception License_processing_error
-exception Missing_connection_details
-exception License_checkout_error of string


Do we not need License_checkout_error anymore? It's not there in type errors.

robhoes · 2018-01-15T13:42:23Z

v6/v6_interface.ml

+  | License_processing_error
+  (** License could not be processed *)
+  | Missing_connection_details
+  (** Thrown if connection port or address parameter not supplied to check_license *)


check_license isn't part of the interface. I think that this should be apply_edition, which needs such parameters in some cases.

robhoes · 2018-01-15T13:48:45Z

v6/v6_interface.ml

+      namespace = None;
+      description = [
+        "This interface is used by Xapi and V6d to manage ";
+        "XenServer edition licensing of hosts.";


I'd prefer something like: "This interface is used by xapi and v6d to manage enabling and disabling features". Note that this interface is not restricted to XenServer (it's part of the xapi project). And it's also used for things such as controlling experimental features.

robhoes · 2018-01-15T13:58:37Z

v6/v6_interface.ml

+  let apply_edition =
+    declare
+    "apply_edition"
+    ["Checks license info and ensures provided features are compatible."]


I'd prefer something like "checks whether the given edition can be applied, and if successful, returns a list of enabled features and other details".

robhoes · 2018-01-15T14:19:39Z

v6/v6_interface.ml

@@ -67,12 +73,18 @@ type errors =
  (** Thrown by license_check when expiry date matches or precedes current date *)
  | License_processing_error
  (** License could not be processed *)
+  | License_checkout_error of string
+  (** License could not be checked out *)


Ah, there it is ;)

robhoes · 2018-01-15T14:22:20Z

v6/v6_interface.ml

-    "apply_edition"
+    let edition_p = Param.mk ~description:["Edition tite"] Types.string in
+    let edition_info_p = Param.mk ~description:["Edition info"] edition_info in 
+    let current_params_p = Param.mk ~description:["Xapi paramaters"] stringPairLst in


"parameters"

oh god I fixed that in one version and must have lost it among the many rebases. I'll go hang my head in shame now

robhoes · 2018-01-15T14:23:09Z

v6/v6_interface.ml

@@ -56,6 +58,10 @@ type edition_info = {
 }
 [@@deriving rpcty]

+type stringPairLst = (string * string) list


We tend_to_use_underscores_to_separate_words ratherThanCamelCase.

robhoes · 2018-01-15T14:27:24Z

v6/v6_interface.ml

-      with low numbers corresponding to editions with fewer features, and vice versa *)
-}
+  {
+    ed_title : string ;


Do we really need these prefixes? I generally prefer to wrap a type in a module instead. In this case, this type is already in module V6_interface and the field names don't seem to conflict with anything else in here?

They were originally necessary to prevent namespace clashes in large functions with an indiscriminate let open V6_interface in , but now all functions using them are wrapped in the module namespace, so I can remove the prefixes.

robhoes · 2018-01-15T15:26:40Z

v6/v6_interface.ml

+(* --- API interface --- *)
+
+(** Generic licensing error of error variant type *)
+exception LicensingError of errors


V6_error perhaps?

coveralls · 2018-01-17T15:16:43Z

Coverage remained the same at 54.268% when pulling 14247eb on minishrink:ppx into 144fa27 on xapi-project:master.

coveralls · 2018-01-18T15:59:12Z

Coverage remained the same at 54.268% when pulling decb961 on minishrink:ppx into 2b50792 on xapi-project:master.

coveralls · 2018-01-19T09:44:21Z

Coverage remained the same at 54.268% when pulling 47160fe on minishrink:ppx into 0d8714a on xapi-project:master.

[v6d] Define functor to generate RPC code Note: Functor was originally just called API, changed this to prevent namespace clashes with an autogenerated module titled API [v6d] Type functions and parameters for PPX code generation

includes the following: - refactor error handling - rename record fields and functor to prevent namespace clashes - edit docstrings Note: Types require a docstring above, not below ocp-indent [v6d] Define error handling, rename record fields and functor to avoid namespace clashes Edit v6 interface docstrings Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com> CP-26098: Wrap matcher and raiser in parentheses Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>

- fix typo (paramaters -> parameters) - rename type (stringPairLst -> string_pair_lst) - rename edition record field names (ed_title -> title etc) - edit docstrings to accurately describe feature handling - rename exception (LicensingError -> V6d_error)

Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>

lindig reviewed Jan 8, 2018

View reviewed changes

minishrink mentioned this pull request Jan 9, 2018

CP-26098: Upgrade xapi to use PPX-based v6d interface xapi-project/xen-api#3391

Merged

mseri requested changes Jan 9, 2018

View reviewed changes

minishrink force-pushed the ppx branch 2 times, most recently from f5cc77f to b917654 Compare January 10, 2018 11:22

lindig approved these changes Jan 11, 2018

View reviewed changes

mseri added the don't merge yet label Jan 11, 2018

robhoes requested changes Jan 15, 2018

View reviewed changes

robhoes reviewed Jan 15, 2018

View reviewed changes

minishrink force-pushed the ppx branch from b917654 to 14247eb Compare January 17, 2018 15:00

minishrink force-pushed the ppx branch from 14247eb to decb961 Compare January 18, 2018 15:41

minishrink force-pushed the ppx branch from decb961 to dd807d4 Compare January 19, 2018 09:27

minishrink force-pushed the ppx branch from dd807d4 to 78f8de8 Compare January 21, 2018 10:29

Akanksha Mathur added 5 commits January 22, 2018 14:48

CP-26098: Update jbuild file

d5562ba

CP-26098: Update V6 client error handling for PPX 2

2860342

minishrink force-pushed the ppx branch from 78f8de8 to 90b352d Compare January 22, 2018 16:49

CP-26098: Autogen CLI for dev/debugging

47160fe

Signed-off-by: Akanksha Mathur <akanksha.mathur@citrix.com>

minishrink force-pushed the ppx branch from 90b352d to 47160fe Compare January 22, 2018 16:50

robhoes merged commit 79df187 into xapi-project:master Jan 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CP-26098: Port v6d interface from Camlp4 to PPX #186

CP-26098: Port v6d interface from Camlp4 to PPX #186

minishrink commented Jan 8, 2018 •

edited

Loading

coveralls commented Jan 8, 2018 •

edited

Loading

lindig left a comment

lindig Jan 8, 2018

minishrink Jan 8, 2018

lindig Jan 8, 2018

minishrink Jan 8, 2018 •

edited

Loading

mseri Jan 9, 2018

lindig Jan 8, 2018

minishrink commented Jan 8, 2018

lindig commented Jan 8, 2018

coveralls commented Jan 8, 2018 •

edited

Loading

minishrink commented Jan 8, 2018 •

edited

Loading

mseri left a comment

mseri Jan 9, 2018

mseri Jan 9, 2018

coveralls commented Jan 10, 2018 •

edited

Loading

lindig commented Jan 11, 2018

robhoes commented Jan 15, 2018

robhoes left a comment

robhoes Jan 15, 2018

robhoes Jan 15, 2018

robhoes Jan 15, 2018

robhoes Jan 15, 2018

robhoes Jan 15, 2018

robhoes Jan 15, 2018

minishrink Jan 15, 2018

robhoes Jan 15, 2018

robhoes Jan 15, 2018

minishrink Jan 15, 2018

robhoes Jan 15, 2018

coveralls commented Jan 17, 2018 •

edited

Loading

coveralls commented Jan 18, 2018 •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading

CP-26098: Port v6d interface from Camlp4 to PPX #186

CP-26098: Port v6d interface from Camlp4 to PPX #186

Conversation

minishrink commented Jan 8, 2018 • edited Loading

coveralls commented Jan 8, 2018 • edited Loading

lindig 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

minishrink Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minishrink commented Jan 8, 2018

lindig commented Jan 8, 2018

coveralls commented Jan 8, 2018 • edited Loading

minishrink commented Jan 8, 2018 • edited Loading

mseri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 10, 2018 • edited Loading

lindig commented Jan 11, 2018

robhoes commented Jan 15, 2018

robhoes 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

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

coveralls commented Jan 17, 2018 • edited Loading

coveralls commented Jan 18, 2018 • edited Loading

coveralls commented Jan 19, 2018 • edited Loading

minishrink commented Jan 8, 2018 •

edited

Loading

coveralls commented Jan 8, 2018 •

edited

Loading

minishrink Jan 8, 2018 •

edited

Loading

coveralls commented Jan 8, 2018 •

edited

Loading

minishrink commented Jan 8, 2018 •

edited

Loading

coveralls commented Jan 10, 2018 •

edited

Loading

coveralls commented Jan 17, 2018 •

edited

Loading

coveralls commented Jan 18, 2018 •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading