CA-245333: changes to facilitate autogeneration of historical releases in API derivatives #3038

kc284 · 2017-05-15T08:33:39Z

The first commit is minor refactoring. The second is needed so the SDK can autogenerate the releases without us having to re-map manually release code names to API versions and server versions. The third commit is not strictly necessary for this ticket, but it can save us some trouble (as long as it doesn't conflict with existing plans regarding the docs).
Please double check the mapping is correct, as the API version has not always been bumped between releases.

kc284 · 2017-05-16T08:27:07Z

This PR has to be merged together with xapi-project/xen-api-sdk#95 to prevent build breakage.

lindig

Some small improvements needed when working with files.

lindig · 2017-05-16T10:10:00Z

ocaml/doc/jsapi.ml

+  let templ = Mustache.of_string (String.concat "\n" lines_read) in
+  let rendered = Mustache.render templ json in
+  let out_chan = open_out output_file in
+  finally (fun () -> Printf.fprintf out_chan "%s" rendered)


This finally does not catch any problems with open_out or other failures in the lines above. To emit a string to a file, use output_string:

val Pervasives.output_string: out_channel -> string -> unit

lindig · 2017-05-16T10:13:25Z

ocaml/doc/jsapi.ml

+let populate_releases templates_dir dest_dir=
+  let infile x = Filename.concat templates_dir x in
+  let outfile x = Filename.concat dest_dir x in
+  let render x = render_template (infile (fst x)) json_releases (outfile (snd x)) in


When x is a pair, it would be better to define let render (infile, outfile) = and to use infile and outfile. Does it have to be a pair? These could be also passed as separate parameters.

It doesn't have to be a pair, I just thought it's convenient for piping a list of input-output file pairs.

lindig · 2017-05-16T10:18:54Z

ocaml/idl/dm_api.ml

    let rec getlast l = (* TODO: move to standard library *)
-      match l with [x] -> x | _::xs -> getlast xs | [] -> raise (Invalid_argument "getlast") in
+      match l with
+      | [x] -> x.code_name


I think it's not a good idea to mix a standard function (getlast - usually called last) with something specific here. I'd keep getlast and work with the result at the call site:

getlast xs |> fun x -> x.code_name

gaborigloi · 2017-05-16T11:07:26Z

I think we should also add mustache as an opam dependency to the opam file in this repo, and to xapi's opam file in xs-opam, since this opam package is now required for building xapi.

kc284 · 2017-05-16T11:32:44Z

@stephen-turner and/or @jonludlam could you please double check I've got the version mapping in the datamodel_types right, as there are some small inconsistencies with what we had in https://github.com/xapi-project/xen-api-sdk/blob/master/csharp/src/Helper.cs#L38

stephen-turner · 2017-05-16T11:56:16Z

I'm fairly confident the ones in the current SDK are correct.

I notice you sometimes have two different "releases" with the same API version: the SDK wouldn't know about those, of course.

kc284 · 2017-05-16T11:58:59Z

These are filtered out, please compare to xapi-project/xen-api-sdk#95

mseri · 2017-05-18T16:31:42Z

ocaml/doc/jsapi.ml

+
+let render_template template_file json output_file =
+  let lines_read = Stdext.Unixext.read_lines template_file in
+  let templ = Mustache.of_string (String.concat "\n" lines_read) in


This could be

let templ = Stdext.Unixext.string_of_file template_file |> Mustache.of_string in

This writes directly to the string, will avoid creating a list and then concatenating it.

mseri · 2017-05-18T16:36:21Z

ocaml/doc/jsapi.ml

+  let lines_read = Stdext.Unixext.read_lines template_file in
+  let templ = Mustache.of_string (String.concat "\n" lines_read) in
+  let rendered = Mustache.render templ json in
+  let out_chan = open_out output_file in


I think the safest is

finally (fun () -> output_string out_chan rendered) (fun () -> close_out out_chan)

I don't think output_string closes the file on error.

robhoes · 2017-05-18T16:39:47Z

ocaml/idl/datamodel_types.ml

+      code_name     = Some rel_rio;
+      version_major = 1;
+      version_minor = 1;
+      branding      = "XCP 0.5";


This isn't quite right: there was no XCP before Midnight Ride. In fact, I think we should just remove the special XCP branding entirely, and have just one string, like on http://xapi-project.github.io/xen-api/releases.html. We can just keep a single version of the docs on ocaml/doc, since XCP doesn't exist anymore.

Ah ok, I didn't know rio was wrong, I simply copied it from branding.js. Good to hear it can go, this means we don't need to package 2 docs versions in the server any more.

robhoes · 2017-05-18T16:42:09Z

ocaml/idl/datamodel_types.ml

+      branding      = "XenServer 7.0";
+      xs_branding   = "XenServer 7.0";
+    }; {
+      code_name     = Some rel_dundee_plus;


I think this is completely unused and should be removed from the datamodel. And if it is used in datamodel, then that should be fixed :)

It is used in the datamodel, for example PVS is under dundee-plus, which means it's too late to change it :(

We should just change those to ely, because that's where they were released, and dundee-plus doesn't exist.

robhoes · 2017-05-18T16:44:09Z

ocaml/idl/datamodel_types.ml

+      code_name     = Some rel_indigo;
+      version_major = 2;
+      version_minor = 4;
+      branding      = "indigo (unreleased)";


I think that this can be called "XenServer 6.5 SP1 Hotfix X", but we have to look up "X"...

coveralls · 2017-05-19T10:32:42Z

Changes Unknown when pulling 07e382e on kc284:master into ** on xapi-project:master**.

Signed-off-by: Konstantina Chremmou <konstantina.chremmou@citrix.com>

coveralls · 2017-05-19T11:49:34Z

Coverage decreased (-0.008%) to 13.98% when pulling c2f2401 on kc284:master into 3cfbf6d on xapi-project:master.

robhoes · 2017-05-19T12:13:34Z

ocaml/idl/datamodel.ml

-let dundee_plus_release =
-  { internal = get_product_releases rel_dundee_plus
+let ely_release =
+  { internal = get_product_releases rel_dundee


Oooops! 😳

Signed-off-by: Konstantina Chremmou <konstantina.chremmou@citrix.com>

…I version and branding info. Thus clients and documentation generated from the API can be udpated automatically without having each time to map manually the release code name to the version or the branding, which is an error prone process Signed-off-by: Konstantina Chremmou <konstantina.chremmou@citrix.com>

…enu. This solution proposes to use mustache templates which can be populated at runtime with the values defined in the datamodel. Thus we avoid duplication of definitions which may as well get out of sync. Also, removed the obsolete XCP branding. Signed-off-by: Konstantina Chremmou <konstantina.chremmou@citrix.com>

robhoes · 2017-05-19T12:31:21Z

ocaml/idl/datamodel.ml

-let dundee_plus_release =
-  { internal = get_product_releases rel_dundee_plus
+let ely_release =
+  { internal = get_product_releases rel_ely


OK :) I'm surprised that we didn't have ely_release before 😕

I can see why it might be forgotten: x_release and rel_x are in different files.

coveralls · 2017-05-19T12:45:13Z

Coverage increased (+0.007%) to 13.994% when pulling 25b42f9 on kc284:master into 3cfbf6d on xapi-project:master.

kc284 added the don't merge label May 15, 2017

kc284 force-pushed the master branch from 10c4542 to f3f4348 Compare May 15, 2017 12:09

kc284 mentioned this pull request May 16, 2017

CA-245333 (api-version autogeneration); plus some small refactoring xapi-project/xen-api-sdk#95

Merged

kc284 removed the don't merge label May 16, 2017

lindig requested changes May 16, 2017

View reviewed changes

kc284 force-pushed the master branch from f3f4348 to 07e382e Compare May 17, 2017 11:03

kc284 added the updated label May 17, 2017

kc284 mentioned this pull request May 17, 2017

CA-245333: Added mustache package to the xapi dependencies. xapi-project/xs-opam#83

Merged

mseri reviewed May 18, 2017

View reviewed changes

robhoes reviewed May 18, 2017

View reviewed changes

Replaced hardcoded values with variables.

9d3a28b

Signed-off-by: Konstantina Chremmou <konstantina.chremmou@citrix.com>

kc284 force-pushed the master branch from 07e382e to c2f2401 Compare May 19, 2017 11:27

robhoes reviewed May 19, 2017

View reviewed changes

Konstantina Chremmou added 3 commits May 19, 2017 13:21

Removed release dundee_plus as it was never released.

4792136

Signed-off-by: Konstantina Chremmou <konstantina.chremmou@citrix.com>

kc284 force-pushed the master branch from c2f2401 to 25b42f9 Compare May 19, 2017 12:23

robhoes reviewed May 19, 2017

View reviewed changes

robhoes approved these changes May 19, 2017

View reviewed changes

mseri approved these changes May 19, 2017

View reviewed changes

kc284 merged commit 4691dca into xapi-project:master May 19, 2017

CA-245333: changes to facilitate autogeneration of historical releases in API derivatives #3038

CA-245333: changes to facilitate autogeneration of historical releases in API derivatives #3038

Uh oh!

Conversation

kc284 commented May 15, 2017

Uh oh!

kc284 commented May 16, 2017

Uh oh!

lindig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborigloi commented May 16, 2017

Uh oh!

kc284 commented May 16, 2017

Uh oh!

stephen-turner commented May 16, 2017

Uh oh!

kc284 commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kc284 May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 19, 2017

Uh oh!

coveralls commented May 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 19, 2017

Uh oh!

Uh oh!

mseri May 18, 2017 •

edited

Loading

kc284 May 19, 2017 •

edited

Loading