Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate Golang codes for SDK #5540

Merged
merged 14 commits into from
Apr 18, 2024

Conversation

duobei
Copy link
Contributor

@duobei duobei commented Mar 29, 2024

  1. generate Golang code of Record and Ref Type
  2. generate Golang code of Enum Type
  3. generate file headers
  4. generate api messages and errors

@duobei duobei force-pushed the private/fezhan/CP-48666 branch 2 times, most recently from ac0a8ed to 30434a1 Compare March 29, 2024 13:04
@duobei duobei changed the base branch from master to feature/go_sdk March 29, 2024 13:18
@duobei duobei force-pushed the private/fezhan/CP-48666 branch from 30434a1 to 3110ed1 Compare April 1, 2024 01:48
ocaml/sdk-gen/go/README.md Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/dune Outdated Show resolved Hide resolved
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two big issues I've seen:

  • I haven't seen any justification for adding the file ocaml/sdk-gen/go/go_file_headers.ml, it looks like it should be instead autogenerated from the datamodel and not committed.

  • All the api messages have changed and this causes a lot of churn. I understand you want to have a way to collect all of them and expose them. Don't other language SDKs do this? If not we can look for other ways of doing this (maybe a ppx?) the method should be reused for all other alnguages

ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/test_gen_go.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/go_file_headers.ml Outdated Show resolved Hide resolved
ocaml/xapi-consts/api_errors.ml Show resolved Hide resolved
quality-gate.sh Outdated Show resolved Hide resolved
@duobei duobei force-pushed the private/fezhan/CP-48666 branch from fcf2dad to 944f300 Compare April 3, 2024 12:46
@minglumlu
Copy link
Member

minglumlu commented Apr 7, 2024

It seems there are some duplications between ocaml/sdk-gen/go/gen_go_helper.ml and ocaml/idl/json_backend/gen_json.ml. Is it possible to eliminate these duplications? What's more, could the generated JSON be a language-neutral one? So that we only need to focus on the transformations from JSON to go-specific.

Update:
Ah, I see the differences between Yojson and mustache JSON. Could they be converted to each other easily? At least other SDKs are using mustache JSON as well. Could the Yojson be replaced with mustache JSON?

@duobei duobei force-pushed the private/fezhan/CP-48666 branch from 944f300 to f68bfd7 Compare April 8, 2024 07:25
@kc284 kc284 requested review from kc284 and danilo-delbusso April 8, 2024 07:56
@duobei duobei force-pushed the private/fezhan/CP-48666 branch from 1b8a051 to 455d344 Compare April 8, 2024 12:36
@duobei
Copy link
Contributor Author

duobei commented Apr 8, 2024

Two big issues I've seen:

  • I haven't seen any justification for adding the file ocaml/sdk-gen/go/go_file_headers.ml, it looks like it should be instead autogenerated from the datamodel and not committed.
  • All the api messages have changed and this causes a lot of churn. I understand you want to have a way to collect all of them and expose them. Don't other language SDKs do this? If not we can look for other ways of doing this (maybe a ppx?) the method should be reused for all other alnguages

File heders are autogenerated now.
I have split api_errors.ml modifications into its own commit.

ocaml/sdk-gen/go/dune Outdated Show resolved Hide resolved
@duobei duobei force-pushed the private/fezhan/CP-48666 branch 4 times, most recently from 5f004f2 to 32e165a Compare April 9, 2024 06:41
@duobei
Copy link
Contributor Author

duobei commented Apr 9, 2024

It seems there are some duplications between ocaml/sdk-gen/go/gen_go_helper.ml and ocaml/idl/json_backend/gen_json.ml. Is it possible to eliminate these duplications? What's more, could the generated JSON be a language-neutral one? So that we only need to focus on the transformations from JSON to go-specific.

Update: Ah, I see the differences between Yojson and mustache JSON. Could they be converted to each other easily? At least other SDKs are using mustache JSON as well. Could the Yojson be replaced with mustache JSON?

In the beginning, I wanted to reuse gen_json.ml. As development progressed, I abandoned that idea.
Main reasons below:

  1. The mustache templates decide the generated JSON. Templates vary widely between different languages.
  2. At its core, OCaml and various language data types are converted to each other by some recursion functions.
    Those functions are not shared.

Surprisingly, the transition between Yojson and mustache JSON is simple through pattern matching.

ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
@duobei duobei force-pushed the private/fezhan/CP-48666 branch from abf107a to 05d07ed Compare April 10, 2024 06:47
@duobei duobei force-pushed the private/fezhan/CP-48666 branch from 90c2764 to 09fb034 Compare April 15, 2024 05:23
duobei added 9 commits April 15, 2024 13:40
CP-47350: Add mustache template for Record/Ref/Class Types
CP-47365: Add mustache template for API messages/errors
CP-47363: Add mustache template for file header

Signed-off-by: Luca Zhang <[email protected]>
@duobei duobei force-pushed the private/fezhan/CP-48666 branch from 09fb034 to 69da32f Compare April 15, 2024 05:40
@kc284
Copy link
Contributor

kc284 commented Apr 17, 2024

Not a blocker for this PR, it can go in a new one) but it may be a good idea at some point to add a sanity build in .github/workflows/generate-and-build-sdks.yml to ensure that if people make future API changes they won't break the autogenerated Go code (we're in the process of adding them for the other SDKs too, e.g. see PR#5555).

@duobei
Copy link
Contributor Author

duobei commented Apr 17, 2024

Not a blocker for this PR, it can go in a new one) but it may be a good idea at some point to add a sanity build in .github/workflows/generate-and-build-sdks.yml to ensure that if people make future API changes they won't break the autogenerated Go code (we're in the process of adding them for the other SDKs too, e.g. see PR#5555).

Cool. We will add that in the following PR.

ocaml/sdk-gen/go/test_gen_go.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_binding.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_helper.ml Outdated Show resolved Hide resolved
ocaml/sdk-gen/go/gen_go_helper.mli Outdated Show resolved Hide resolved
duobei added 5 commits April 18, 2024 11:20
Before we got the enums alongside objs,  the readability of code is so poor. We separate to get them now.

Signed-off-by: Luca Zhang <[email protected]>
Just check the fields only we need instead of generalized recursive functions

Signed-off-by: Luca Zhang <[email protected]>
@duobei duobei force-pushed the private/fezhan/CP-48666 branch from 69da32f to 1351856 Compare April 18, 2024 05:14
@duobei duobei merged commit 25df1a8 into xapi-project:feature/go_sdk Apr 18, 2024
13 checks passed
Copy link

pytype_reporter extracted 50 problem reports from pytype output

.

You can check the results of the job here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants