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

API Generator #177

Merged
merged 2 commits into from
Aug 9, 2023
Merged

API Generator #177

merged 2 commits into from
Aug 9, 2023

Conversation

nhtruong
Copy link
Collaborator

Description

Added API Generator to generate API actions from OpenSearch spec.

Issues Resolved

closes #139

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.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This is very good. I would try harder to externalize anything hard-coded like lists of API methods, and I think we still need:

  1. An (easier) way to run the generator via rake generate.
  2. A CI/CD that fails when the APIs are out of date.
  3. A cron that updates the APIs, ala https://github.com/slack-ruby/slack-ruby-client/blob/master/.github/workflows/update_api.yml

api_generator/lib/api_generator.rb Outdated Show resolved Hide resolved
@nhtruong
Copy link
Collaborator Author

nhtruong commented Jul 27, 2023

@dblock Agree with all 3. They are actually in the roadmap. We will have follow-up PRs to add those features. For now, this is to be done manually so that the Ruby client can get the new endpoints added easily.

Signed-off-by: Theo Truong <[email protected]>
…senting namespaces. We don't need to hardcode existing namespaces anymore!

Signed-off-by: Theo Truong <[email protected]>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Some nits on docs and an initializer, I skimmed the rest, I think it's good enough and we should move forward.

- Download the latest OpenSearch API Specification from [The API Spec Repo](https://github.com/opensearch-project/opensearch-api-specification/blob/main/OpenSearch.openapi.json)
- Run the generator with the API Spec downloaded previously (see below)
- Run Rubocop with `-a` flag to remove redundant spacing from the generated code `rubocop -a`
- Commit and create a PR to merge the updated API actions into `main`.
Copy link
Member

Choose a reason for hiding this comment

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

Nit, copy-paste commands to do this as an example so people can "just do it".

- Commit and create a PR to merge the updated API actions into `main`.

### Generate API Actions
Install all dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Do I have to actually do this somewhere or is it a doc on how the generator works? Update text.

:body, :body_description, :body_required

# @param [Array<Operation>] operations
def initialize(operations)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I generally would prefer that initialize just assigns @operations and to expose all the other things as properties that use each-other, this way you could document what they mean.

There's also some assumption here that there's only 1 operation?

attr_reader :operations

def initialize(operations)
  @operations = operations
  raise "Missing operations." if operations.size == 0
  raise "Multiple operations not supported." if operations.size > 1
end

# name of the first operation
def name
   operations&.first&.name
end   

Copy link
Collaborator Author

@nhtruong nhtruong Aug 9, 2023

Choose a reason for hiding this comment

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

No there are multiple operations in 1 action, but they all share the group property, which is then broken into name and namespace

@nhtruong
Copy link
Collaborator Author

nhtruong commented Aug 9, 2023

@dblock Will address these in a follow-up PR. I'm going to switch over getting the security API generated after this is merged.

Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

This looks great @nhtruong!

Can we also list down the next steps/improvements in a new issue or may be in the existing one #139?

@dblock dblock merged commit 55fb48d into opensearch-project:main Aug 9, 2023
62 checks passed
@nhtruong nhtruong deleted the api_generator branch August 9, 2023 23:00
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.

[FEATURE] Generate API code from OpenSearch API Spec
3 participants