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

[OIL] Use mustache templates to build Java SDK + Add Java SDK build step to actions #5595

Merged
merged 9 commits into from
Sep 13, 2024

Conversation

danilo-delbusso
Copy link
Member

@danilo-delbusso danilo-delbusso commented Apr 26, 2024

Unfortunately, I couldn't come up with an easy way to keep the same order of types in Types.java. The issue here is that they were added as they were found, so there was no specified order.

A good way to check would probably be to compare class structures instead. I'm happy to assist if possible.

The SDK sources are present in the PR's artifacts so you don't have to build them yourself.

This PR also adds the JAR to the release files.

Note that I did keep the existing side-effect operations of updating enums, types, etc.. I found that they weren't that much of an hassle to keep but feel free to weigh in.

For result of a release with the changes here see https://github.com/danilo-delbusso/xen-api/releases/tag/v60.1.3

@danilo-delbusso
Copy link
Member Author

force push was a rebase on master to fix conflicts with #5610

I also added 36d9134 as part of the push

ocaml/sdk-gen/java/templates/Class.mustache Outdated Show resolved Hide resolved
ocaml/sdk-gen/java/templates/Class.mustache Outdated Show resolved Hide resolved
ocaml/sdk-gen/java/templates/Class.mustache Outdated Show resolved Hide resolved
.github/workflows/release.yml Show resolved Hide resolved
Copy link
Contributor

@kc284 kc284 left a comment

Choose a reason for hiding this comment

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

As above. It wil also need rebasing due to conflicts.

@danilo-delbusso
Copy link
Member Author

force push is a rebase on master to fix merge conflicts

Also adds a better definition of the compare function for module Ty. The previous one was leaving duplicates in the set by relying on the underlying `Set.compare`.

Signed-off-by: Danilo Del Busso <[email protected]>
Signed-off-by: Danilo Del Busso <[email protected]>
Also update wording in `parseResult`

Signed-off-by: Danilo Del Busso <[email protected]>
After the rebase the change regressed

Signed-off-by: Danilo Del Busso <[email protected]>
Signed-off-by: Danilo Del Busso <[email protected]>
* The value does not belong to this enumeration
*/
@JsonEnumDefaultValue
UNRECOGNIZED,
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we use "UNRECOGNIZED" as default value for Jave SDK , can we use same default Enum value for Go #5981? @kc284

Copy link
Contributor

@kc284 kc284 Sep 13, 2024

Choose a reason for hiding this comment

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

This is similar to the unknown_enum_value added in the Go PR. The result is that there are many enums (e.g. LatestSyncedUpdatesAppliedState or SriovConfigurationMode etc.) that have both unknown and unrecognized and I find this confusing from the client's perspective. I think the best way to handle it is like C# where we add unknown, but only for the enums that do no already have unknown.

@kc284 kc284 merged commit 373672b into xapi-project:master Sep 13, 2024
15 checks passed
@danilo-delbusso danilo-delbusso deleted the oil/java branch September 13, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants