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

Update OpenAPI spec #160

Merged
merged 4 commits into from
Oct 2, 2023
Merged

Update OpenAPI spec #160

merged 4 commits into from
Oct 2, 2023

Conversation

mgyucht
Copy link
Contributor

@mgyucht mgyucht commented Oct 2, 2023

Changes

Update of the OpenAPI spec. This also introduces the template_libraries field being added in databricks/databricks-sdk-go#635.

This PR contains some other changes:

  1. Change float/Float to double/Double. OpenAPI specs currently specify format: "double" for all non-integral numeric values.
  2. Remove old types that were previously in the OpenAPI spec but are no longer needed.
  3. Do not generate files for types that don't correspond to distinct Java types (e.g. are not objects or enums). A lot of extra files are generated unnecessarily today, corresponding to either integers/strings/booleans or arrays/maps. Since Java has no type-def functionality, these are represented in the SDK as their JDK equivalents.

Tests

Added AccountMetastoreAssignmentsIT to verify the correct functionality of the updated AccountMetastoreAssignments API.
Nightly tests passed on this PR.

@mgyucht mgyucht changed the title [WIP] Update OpenAPI spec Update OpenAPI spec Oct 2, 2023
@mgyucht mgyucht requested a review from tanmay-db October 2, 2023 12:22
{{- define "type-unboxed" -}}
{{- if .IsBool}}boolean
{{- else if .IsInt64}}long
{{- else if .IsFloat64}}double
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch

{{ else }}
Any types that have no fields (i.e. primitives or array types) or are not enums are not represented by distinct Java
types in the Java SDK.
{{skipThisFile}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This would prevent generation of WorkspaceId.java and other files right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. That's why several files disappear from the .gitattributes file.

.codegen.json Outdated
@@ -19,6 +22,7 @@
},
"toolchain": {
"require": ["mvn", "java"],
"setup": ["rm -rf databricks-sdk-java/src/main/java/com/databricks/sdk/service"],
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for? Do we want to use the force flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, we can just use -r.

* string of utf-8 characters. The value can be an empty string, with maximum length of 255
* characters. The key can be of maximum length of 127 characters, and cannot be empty.
*/
// type CustomTags Map<String,String>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these were never used, this change shouldn't be breaking right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

@mgyucht mgyucht enabled auto-merge October 2, 2023 15:29
@mgyucht mgyucht added this pull request to the merge queue Oct 2, 2023
Merged via the queue into main with commit 5092481 Oct 2, 2023
9 checks passed
@mgyucht mgyucht deleted the update-openapi-spec-2-oct-2023 branch October 2, 2023 15:33
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.

2 participants