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

Add cluster version type #187

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

samuelorji
Copy link
Contributor

Description

Add a concrete type for the cluster version info, such that clients can parse the json like this:

client.info().send().await?.json::<ClusterInfo>().await?

But according to this, we shouldn't modify the file, so what will be the best way to go about it. Do we add this to the code generation ? . And if we do, do we create a module where we store the structure of possible typed responses, thereby setting a foundation for more type safety ?

Issues Resolved

#94

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #187 (90db4b0) into main (5a70fe5) will decrease coverage by 0.05%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
- Coverage   73.86%   73.82%   -0.05%     
==========================================
  Files         401      402       +1     
  Lines       63702    63733      +31     
==========================================
- Hits        47055    47049       -6     
- Misses      16647    16684      +37     
Flag Coverage Δ
integration 73.82% <ø> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 13 files with indirect coverage changes

@dblock
Copy link
Member

dblock commented Oct 2, 2023

There's some mention of generating APIs from spec in https://github.com/opensearch-project/opensearch-rs/blob/main/DEVELOPER_GUIDE.md#cargo-make. I think we need something more thorough that explains how code generation works, where the templates are, etc.

@samuelorji
Copy link
Contributor Author

There's some mention of generating APIs from spec in https://github.com/opensearch-project/opensearch-rs/blob/main/DEVELOPER_GUIDE.md#cargo-make. I think we need something more thorough that explains how code generation works, where the templates are, etc.

Thanks for the review, do you think it's better to add concrete types to the code generator, or make it static in the opensearch module ?

@samuelorji
Copy link
Contributor Author

@dblock , I'm thinking of adding a models.rs file to the opensearch where concrete json response types can live. Will that be a good idea

@dblock
Copy link
Member

dblock commented Oct 3, 2023

I don't know enough about this to answer @samuelorji, but I bet @Xtansia does?

@Xtansia
Copy link
Collaborator

Xtansia commented Oct 3, 2023

The generator will need at least 2 stages, first modifying the existing generator to read OpenAPI specs rather than legacy ones, then modify generator to generate request/response bodies. The generation of req/resp bodies is somewhat distant due to the spec not-yet containing all the information for them and we'd want to carefully consider and design how we'd tie the body types to the request actions.

As such I think it's fine to add some hand-written body structures now that users can take advantage of if it improves DX sooner, we'll just need to put them in separate files (may want to re-organize the layout into folders?) and modify the generator template to re-export the types out of the correct module (via pub use)

@samuelorji samuelorji force-pushed the feature/add-info-object branch from b827233 to cf7e5a1 Compare October 5, 2023 08:55
@samuelorji samuelorji marked this pull request as ready for review October 5, 2023 08:55
@samuelorji samuelorji changed the title draft: add cluster version type Add cluster version type Oct 5, 2023
@@ -37,6 +37,7 @@ pub mod headers;
pub mod request;
pub mod response;
pub mod transport;
pub mod models;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be inside the http module, as it's not specific to the http implementation/transport layer.

I'd maybe lay it out something like:

opensearch
|- src
    |- models
        |- mod.rs

Because then we can also eventually put namespace specific models like for cluster etc under models


#[derive(Deserialize, Debug)]
#[doc = "Cluster information"]
pub struct ClusterInfo {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename this to InfoResponse so it's a bit more obvious what action it relates to.

version: ClusterVersionInfo,
#[serde(rename = "tagline")]
tag_line: String

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this blank line

}

#[derive(Deserialize, Debug)]
pub struct ClusterVersionInfo {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most other clients (and server) refer to this as OpenSearchVersionInfo

@samuelorji samuelorji force-pushed the feature/add-info-object branch from cf7e5a1 to 8fce8f9 Compare October 11, 2023 12:24
@samuelorji samuelorji requested a review from Xtansia October 11, 2023 12:24
Copy link
Collaborator

@Xtansia Xtansia left a comment

Choose a reason for hiding this comment

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

Looks good, you just need to add a line to the CHANGELOG.md file under the ### Added header, following the format of the other lines ie. - Added InfoResponse structure ([#pr](https://....))

@samuelorji samuelorji force-pushed the feature/add-info-object branch from eb1e043 to d1f5c29 Compare October 11, 2023 20:17
@samuelorji samuelorji requested a review from Xtansia October 11, 2023 20:19
@samuelorji samuelorji force-pushed the feature/add-info-object branch from d1f5c29 to 90db4b0 Compare October 11, 2023 20:24
@Xtansia Xtansia merged commit 94e7b72 into opensearch-project:main Oct 11, 2023
29 of 31 checks passed
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.

3 participants