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

adding specs for SQL namespace #379

Merged
merged 28 commits into from
Jul 25, 2024
Merged

Conversation

Tokesh
Copy link
Collaborator

@Tokesh Tokesh commented Jul 7, 2024

Description

adding sql query specification

Issues Resolved

[#235]

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
Contributor

github-actions bot commented Jul 7, 2024

Changes Analysis

Commit SHA: 1573d8b
Comparing To SHA: 592b9b6

API Changes

Summary


├─┬Paths
│ ├──[➕] path (11731:3)
│ ├──[➕] path (5650:3)
│ ├──[➕] path (11777:3)
│ ├──[➕] path (5671:3)
│ ├──[➕] path (5692:3)
│ ├──[➕] path (5629:3)
│ ├──[➕] path (11754:3)
│ └──[➕] path (11708:3)
└─┬Components
  ├──[➕] requestBodies (23511:7)
  ├──[➕] requestBodies (23523:7)
  ├──[➕] requestBodies (23505:7)
  ├──[➕] requestBodies (23517:7)
  ├──[➕] responses (26294:7)
  ├──[➕] responses (26300:7)
  ├──[➕] responses (26306:7)
  ├──[➕] responses (26288:7)
  ├──[➕] responses (26282:7)
  ├──[➕] parameters (21325:7)
  ├──[➕] parameters (21310:7)
  ├──[➕] parameters (21348:7)
  ├──[➕] parameters (21303:7)
  ├──[➕] parameters (21340:7)
  ├──[➕] parameters (21318:7)
  ├──[➕] parameters (21295:7)
  ├──[➕] parameters (21355:7)
  ├──[➕] parameters (21333:7)
  ├──[➕] parameters (21288:7)
  ├──[➕] schemas (48861:7)
  ├──[➕] schemas (48885:7)
  ├──[➕] schemas (48850:7)
  ├──[➕] schemas (48913:7)
  ├──[➕] schemas (48871:7)
  ├──[➕] schemas (48880:7)
  ├──[➕] schemas (48866:7)
  └──[➕] schemas (48894:7)

Document Element Total Changes Breaking Changes
paths 8 0
components 27 0
  • Total Changes: 35
  • Additions: 35

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10097617017/artifacts/1740354812

API Coverage

Before After Δ
Covered (%) 490 (47.99 %) 500 (48.97 %) 10 (0.98 %)
Uncovered (%) 531 (52.01 %) 521 (51.03 %) -10 (-0.98 %)
Unknown 24 24 0

@Tokesh Tokesh changed the title added sql query specification adding SQL namespace Jul 7, 2024
@Tokesh Tokesh changed the title adding SQL namespace adding specs for SQL namespace Jul 7, 2024
@dblock
Copy link
Member

dblock commented Jul 7, 2024

Tokesh added 11 commits July 8, 2024 19:52
Signed-off-by: Tokesh <[email protected]>
Signed-off-by: Tokesh <[email protected]>
This reverts commit e6958b363dd00b5702dba4159985b935d7360e62.

Signed-off-by: Tokesh <[email protected]>
Signed-off-by: Tokesh <[email protected]>
Signed-off-by: Tokesh <[email protected]>
@Tokesh
Copy link
Collaborator Author

Tokesh commented Jul 8, 2024

Awesome! Please write some tests for it? See https://github.com/opensearch-project/opensearch-api-specification/blob/main/SPECIFICATION_TESTING.md and https://github.com/opensearch-project/opensearch-api-specification/tree/main/tests.

Hello! Yes, i'll add test a bit later. I'm just getting used to OpenAPI.

Tokesh and others added 3 commits July 21, 2024 22:16
Signed-off-by: Niyazbek Torekeldi <[email protected]>
Signed-off-by: Tokesh <[email protected]>
@dblock
Copy link
Member

dblock commented Jul 22, 2024

Want to get this to a mergeable state? It doesn't have to be full coverage of the SQL plugin, can start with just the basics and make more PRs afterwards!

@Tokesh Tokesh marked this pull request as ready for review July 24, 2024 04:03
spec/namespaces/sql.yaml Outdated Show resolved Hide resolved
@nhtruong
Copy link
Collaborator

@dblock That's very unusual for the framework to obscure an error not related to the testing itself. This should not have been handled by the framework and should crash it for the error trace to be shown.

@dblock
Copy link
Member

dblock commented Jul 24, 2024

@dblock That's very unusual for the framework to obscure an error not related to the testing itself. This should not have been handled by the framework and should crash it for the error trace to be shown.

Yes, 100%. I can look into it.

@nhtruong
Copy link
Collaborator

Created an issue: #449

@Tokesh
Copy link
Collaborator Author

Tokesh commented Jul 24, 2024

Probably i found misstake
I'm waiting for the cursor to receive, but it doesn't come with the API
I will test and see

Copy link
Contributor

Spec Test Coverage Analysis

Total Tested
524 118 (22.52 %)

@Tokesh Tokesh requested a review from dblock July 24, 2024 17:19
@Tokesh
Copy link
Collaborator Author

Tokesh commented Jul 24, 2024

I'm so happy! You can check it now!
@dblock @nhtruong

nhtruong
nhtruong previously approved these changes Jul 24, 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.

This is great! Thank you!

@nhtruong
Copy link
Collaborator

Cant merged. Blocked by #379 (review) @dblock

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.

Couple more. I think we should stay disciplined about prologue, sorry to be a hassle.

Also see if you can exercise the parameters like sanitize that aren't.

@@ -35,6 +35,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Added AjvErrorsParser to print more informative error messages ([#364](https://github.com/opensearch-project/opensearch-api-specification/issues/364))
- Added JsonSchemaValidator, a wrapper for AJV ([#364](https://github.com/opensearch-project/opensearch-api-specification/issues/364))
- Added support for `application/cbor` responses ([#371](https://github.com/opensearch-project/opensearch-api-specification/pull/371))
- Added tests for SQL namespace ([#379](https://github.com/opensearch-project/opensearch-api-specification/pull/379))
Copy link
Member

Choose a reason for hiding this comment

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

Actually the add is the spec.

Let's say "Added /_plugins/_sql, close and stats"?

@@ -0,0 +1,53 @@
$schema: ../../json_schemas/test_story.schema.yaml

description: Test to explicitly clear the cursor context
Copy link
Member

Choose a reason for hiding this comment

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

Add a period at the end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Time for another lint!

Copy link
Member

Choose a reason for hiding this comment

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

We have #403, I'll add a note on tests.

parameters:
index: books
response:
status: 200
Copy link
Member

Choose a reason for hiding this comment

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

The above should be moved to a prologue because they aren't exercising the SQL API namespace.

@dblock
Copy link
Member

dblock commented Jul 24, 2024

Probably i found misstake I'm waiting for the cursor to receive, but it doesn't come with the API I will test and see

It did raise that bug of reporting errors, so thank you! Fix in #450.

@Tokesh
Copy link
Collaborator Author

Tokesh commented Jul 25, 2024

Also see if you can exercise the parameters like sanitize that aren't.

I found sanitize in documentation here:
image
And rechecked on local machine

@Tokesh Tokesh requested review from dblock and nhtruong July 25, 2024 16:15
@dblock
Copy link
Member

dblock commented Jul 25, 2024

Also see if you can exercise the parameters like sanitize that aren't.

I found sanitize in documentation here: image And rechecked on local machine

Yes, I meant adding a test that actually uses it.

@dblock dblock merged commit 6fd9afe into opensearch-project:main Jul 25, 2024
14 checks passed
@dblock
Copy link
Member

dblock commented Jul 25, 2024

Merged this! @Tokesh if you want to remove the changelog line that says "Added tests for ... SQL" (I don't think it's helpful), add some tests for ?sanitize and whatever other parts of the spec you declared but didn't test in a next PR, that'd be great!

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.

3 participants