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

Adds API spec coverage report. #179

Merged
merged 5 commits into from
Mar 6, 2024

Conversation

dblock
Copy link
Member

@dblock dblock commented Jan 11, 2024

Description

This report uses the API plugin from https://github.com/dblock/opensearch-api and a tool to show the difference between OpenSearch 2.12 APIs and the OpenAPI spec checked into this repo.

See output in https://github.com/opensearch-project/opensearch-api-specification/actions/runs/8104297882/job/22150630111.

API specs implemented for 232/649 (35%) APIs.

The commented action code can be enabled after this is merged, otherwise it fails with a permissions error.

Issues Resolved

Part of #168.

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
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 is great! We now have a full list of APIs to be added in the specs. Thanks @dblock :)

Does the "Legacy APIs" section mean the APIs that are deprecated?

nhtruong
nhtruong previously approved these changes Jan 12, 2024
@dblock dblock marked this pull request as draft January 17, 2024 17:22
@dblock
Copy link
Member Author

dblock commented Jan 17, 2024

@nhtruong @reta thanks for the review!

I plan to get opensearch-project/OpenSearch#11876 in, move contents of opensearch-project/OpenSearch#11843 into a new plugin (https://github.com/dblock/opensearch-api), publish that to Maven once 2.12 is released, and then use it from here. Stay tuned.

@dblock
Copy link
Member Author

dblock commented Jan 23, 2024

Update:

  1. The supporting code got merged into OpenSearch via Expose RestController all handlers iterator. OpenSearch#11876.
  2. I re-extracted plugin code into https://github.com/dblock/opensearch-api.
  3. This PR was updated to use OpenSearch 2.11.1 + 1 commit and the 2.x branch of opensearch-api.

@dblock dblock force-pushed the api-coverage branch 4 times, most recently from 8c62937 to eec5455 Compare February 29, 2024 23:13
Signed-off-by: dblock <dblock@amazon.com>
@dblock dblock force-pushed the api-coverage branch 7 times, most recently from 175817a to 09e64cb Compare March 1, 2024 00:03
Signed-off-by: dblock <dblock@amazon.com>
@dblock dblock marked this pull request as ready for review March 1, 2024 00:07
@dblock
Copy link
Member Author

dblock commented Mar 1, 2024

@Xtansia @nhtruong take a look, this is ready I think

nhtruong
nhtruong previously approved these changes Mar 4, 2024
Copy link
Collaborator

@nhtruong nhtruong left a comment

Choose a reason for hiding this comment

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

LGTM

I assume that it would be very easy to switch over to comparing yaml specs right?

@dblock
Copy link
Member Author

dblock commented Mar 4, 2024

I assume that it would be very easy to switch over to comparing yaml specs right?

Looks like https://github.com/OpenAPITools/openapi-diff supports YAML and https://kislyuk.github.io/yq/ can replace jq.

Comment on lines 18 to 24
- name: Install API Plugin
run: |
wget https://github.com/dblock/opensearch-api/releases/download/v${{ env.OPENSEARCH_VERSION }}/opensearch-api-${{ env.OPENSEARCH_VERSION }}.0.zip
echo "FROM opensearchproject/opensearch:${{ env.OPENSEARCH_VERSION }}" >> Dockerfile
echo "ADD ./opensearch-api-${{ env.OPENSEARCH_VERSION }}.0.zip /tmp/" >> Dockerfile
echo "RUN /usr/share/opensearch/bin/opensearch-plugin install --batch file:/tmp/opensearch-api-${{ env.OPENSEARCH_VERSION }}.0.zip" >> Dockerfile
cat Dockerfile
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a particular reason to build up the Dockerfile in the workflow rather than committing it and using a build arg to pass in the opensearch version?

Also you could remove the wget and just have opensearch-plugin do the download:

ARG OPENSEARCH_VERSION=2.12.0
FROM opensearchproject/opensearch:${OPENSEARCH_VERSION}
ARG OPENSEARCH_VERSION
RUN /usr/share/opensearch/bin/opensearch-plugin \
    install \
    --batch \
    https://github.com/dblock/opensearch-api/releases/download/v${OPENSEARCH_VERSION}/opensearch-api-${OPENSEARCH_VERSION}.0.zip

Comment on lines 58 to 61
# - uses: mshick/add-pr-comment@v2
# with:
# message: |
# API specs implemented for ${{ steps.coverage.outputs.current }}/${{ steps.coverage.outputs.total }} (${{ steps.math.outputs.percent }}%) APIs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be uncommented or removed?

Copy link
Member Author

@dblock dblock Mar 5, 2024

Choose a reason for hiding this comment

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

The workflow needs permissions which are declared as part of this workflow for this to work. I'll delete it for now.

      # - uses: mshick/add-pr-comment@v2
      #   with:
      #     message: |
      #       API specs implemented for ${{ steps.coverage.outputs.current }}/${{ steps.coverage.outputs.total }} (${{ steps.math.outputs.percent }}%) APIs.

run: |
docker build . --tag opensearch-with-api-plugin
docker run -d -p 9200:9200 -p 9600:9600 -e "discovery.type=single-node" -e OPENSEARCH_INITIAL_ADMIN_PASSWORD="${{ env.OPENSEARCH_INITIAL_ADMIN_PASSWORD }}" opensearch-with-api-plugin
sleep 15
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sleep feels prone to causing spurious errors, but probably good enough for now

Copy link
Member Author

Choose a reason for hiding this comment

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

If you have a better way to wait for service I can change it.

.github/workflows/coverage.yml Outdated Show resolved Hide resolved
.github/workflows/coverage.yml Outdated Show resolved Hide resolved
.github/workflows/coverage.yml Outdated Show resolved Hide resolved
.github/workflows/coverage.yml Outdated Show resolved Hide resolved
.github/workflows/coverage.yml Outdated Show resolved Hide resolved
.github/workflows/coverage.yml Outdated Show resolved Hide resolved
Signed-off-by: dblock <dblock@amazon.com>
@dblock
Copy link
Member Author

dblock commented Mar 5, 2024

@Xtansia Thanks for a thorough review. Took all your suggestions, much simpler now.

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 👍

@nhtruong nhtruong merged commit 21b3e2b into opensearch-project:main Mar 6, 2024
5 checks passed
@dblock dblock deleted the api-coverage branch March 7, 2024 13:26
dlin2028 pushed a commit to dlin2028/security that referenced this pull request May 1, 2024
…#3976)

### Description

Coming from
opensearch-project/opensearch-api-specification#179
which flags a couple of false positives because of mismatched trailing
slash.

### Check List
- [x] New functionality includes testing
- [x] New functionality has been documented
- [x] Commits are signed per the DCO using --signoff

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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

Signed-off-by: dblock <dblock@amazon.com>
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.

None yet

5 participants