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 SQL Settings specs #456

Merged
merged 9 commits into from
Aug 2, 2024

Conversation

Tokesh
Copy link
Collaborator

@Tokesh Tokesh commented Jul 31, 2024

Description

adding sql settings specs

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 31, 2024

Changes Analysis

Commit SHA: 41b888e
Comparing To SHA: b67104c

API Changes

Summary

├─┬Paths
│ ├──[➕] path (11797:3)
│ ├──[➕] path (4074:3)
│ ├─┬/_opendistro/_sql/stats
│ │ ├─┬GET
│ │ │ ├─┬Parameters
│ │ │ │ └─┬Schema
│ │ │ │   └──[➖] description (21323:22)
│ │ │ └─┬Parameters
│ │ │   └─┬Schema
│ │ │     └──[➖] description (21331:22)
│ │ └─┬POST
│ │   ├─┬Parameters
│ │   │ └─┬Schema
│ │   │   └──[➖] description (21346:22)
│ │   └─┬Parameters
│ │     └─┬Schema
│ │       └──[➖] description (21338:22)
│ ├─┬/_plugins/_sql/_explain
│ │ └─┬POST
│ │   ├─┬Parameters
│ │   │ └─┬Schema
│ │   │   └──[➖] description (21308:22)
│ │   └─┬Parameters
│ │     └─┬Schema
│ │       └──[➖] description (21316:22)
│ ├─┬/_opendistro/_sql
│ │ └─┬POST
│ │   ├─┬Parameters
│ │   │ └─┬Schema
│ │   │   └──[➖] description (21361:22)
│ │   └─┬Parameters
│ │     └─┬Schema
│ │       └──[➖] description (21353:22)
│ ├─┬/_plugins/_sql/stats
│ │ ├─┬GET
│ │ │ ├─┬Parameters
│ │ │ │ └─┬Schema
│ │ │ │   └──[➖] description (21323:22)
│ │ │ └─┬Parameters
│ │ │   └─┬Schema
│ │ │     └──[➖] description (21331:22)
│ │ └─┬POST
│ │   ├─┬Parameters
│ │   │ └─┬Schema
│ │   │   └──[➖] description (21338:22)
│ │   └─┬Parameters
│ │     └─┬Schema
│ │       └──[➖] description (21346:22)
│ ├─┬/_plugins/_sql
│ │ └─┬POST
│ │   ├─┬Parameters
│ │   │ └─┬Schema
│ │   │   └──[➖] description (21353:22)
│ │   └─┬Parameters
│ │     └─┬Schema
│ │       └──[➖] description (21361:22)
│ ├─┬/_opendistro/_sql/_explain
│ │ └─┬POST
│ │   ├─┬Parameters
│ │   │ └─┬Schema
│ │   │   └──[➖] description (21316:22)
│ │   └─┬Parameters
│ │     └─┬Schema
│ │       └──[➖] description (21308:22)
│ ├─┬/_opendistro/_sql/close
│ │ └─┬POST
│ │   ├─┬Parameters
│ │   │ └─┬Schema
│ │   │   └──[➖] description (21293:22)
│ │   └─┬Parameters
│ │     └─┬Schema
│ │       └──[➖] description (21301:22)
│ └─┬/_plugins/_sql/close
│   └─┬POST
│     ├─┬Parameters
│     │ └─┬Schema
│     │   └──[➖] description (21293:22)
│     └─┬Parameters
│       └─┬Schema
│         └──[➖] description (21301:22)
└─┬Components
  ├──[➕] requestBodies (23556:7)
  ├──[➕] responses (26271:7)
  ├──[➕] parameters (21395:7)
  ├──[➕] schemas (49051:7)
  ├──[➕] schemas (49025:7)
  ├──[➕] schemas (48950:7)
  ├──[➕] schemas (48925:7)
  ├──[➕] schemas (48941:7)
  ├──[➕] schemas (49015:7)
  ├──[➕] schemas (48909:7)
  ├──[➕] schemas (49056:7)
  ├──[➕] schemas (48934:7)
  └──[➕] schemas (49020:7)

Document Element Total Changes Breaking Changes
paths 22 0
components 13 0
  • Total Changes: 35
  • Removals: 20
  • Additions: 15

Report

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

API Coverage

Before After Δ
Covered (%) 500 (48.97 %) 502 (49.17 %) 2 (0.2 %)
Uncovered (%) 521 (51.03 %) 519 (50.83 %) -2 (-0.2 %)
Unknown 24 24 0

Copy link
Contributor

github-actions bot commented Jul 31, 2024

Spec Test Coverage Analysis

Total Tested
526 117 (22.24 %)

CHANGELOG.md Show resolved Hide resolved
@Tokesh
Copy link
Collaborator Author

Tokesh commented Jul 31, 2024

But i have a question. How i can describe this request body in specs? When user can input 2 different formats of request body. I have already described the second option, where everything is in a more detailed format.
image

tests/sql/settings.yaml Outdated Show resolved Hide resolved
@dblock
Copy link
Member

dblock commented Jul 31, 2024

But i have a question. How i can describe this request body in specs? When user can input 2 different formats of request body. I have already described the second option, where everything is in a more detailed format.

You mean in the schema? Are you looking for - anyOf:?

type: string
size_limit:
type: string
Copy link
Member

Choose a reason for hiding this comment

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

Double-checking that those are actually strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This API accept string and also numbers.
image
image

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.

Let's make sure the types are correct.

properties:
enabled:
type: string
Copy link
Member

Choose a reason for hiding this comment

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

Boolean?

@Tokesh
Copy link
Collaborator Author

Tokesh commented Aug 1, 2024

Let's make sure the types are correct.

I double checked it
image

@Tokesh Tokesh requested review from nhtruong and dblock August 1, 2024 17:58
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.

Thanks! It's close. See comments, look through all unresolved conversations. The types look suspicious.

spec/namespaces/sql.yaml Outdated Show resolved Hide resolved
spec/namespaces/sql.yaml Outdated Show resolved Hide resolved
@Tokesh Tokesh requested a review from dblock August 1, 2024 18:50
@Tokesh
Copy link
Collaborator Author

Tokesh commented Aug 1, 2024

Thanks! It's close. See comments, look through all unresolved conversations. The types look suspicious.

Okkie! I resolved all comments
Also, I attached a few screenshots related to types.

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.

Looks good. One ask for CHANGELOG and we can merge.

spec/namespaces/sql.yaml Show resolved Hide resolved
Signed-off-by: Niyazbek Torekeldi <[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.

Question on boolean:

tests/sql/settings.yaml Outdated Show resolved Hide resolved
@Tokesh Tokesh requested a review from dblock August 2, 2024 18:06
@Tokesh
Copy link
Collaborator Author

Tokesh commented Aug 2, 2024

@dblock @nhtruong ready for your review!

@dblock dblock merged commit 2ca485a into opensearch-project:main Aug 2, 2024
12 checks passed
@Tokesh Tokesh mentioned this pull request Aug 3, 2024
9 tasks
@aabeshov aabeshov mentioned this pull request Aug 9, 2024
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