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.

Sign up for GitHub

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

Jump to bottom

Validate all schemas defined in the spec as valid JSON schemas #294

Merged
merged 1 commit into from
May 17, 2024
Merged

Validate all schemas defined in the spec as valid JSON schemas #294

merged 1 commit into from
May 17, 2024

Conversation

nhtruong
Copy link
Collaborator

  • Updated default arg values for merge.ts
  • Added silencing feature for merger tool
  • Added SchemasValidator

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.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 13, 2024 •
edited
Loading

API specs implemented for 307/649 (47%) APIs.
Commit c6fbbc2.

dblock
dblock reviewed May 14, 2024
View reviewed changes
tools/src/merger/OpenApiMerger.ts Outdated Show resolved Hide resolved
tools/src/merger/SupersededOpsGenerator.ts Outdated Show resolved Hide resolved
@nhtruong nhtruong marked this pull request as draft May 14, 2024 14:53
@nhtruong
Copy link
Collaborator Author

nhtruong commented May 14, 2024 •
edited
Loading

Converting this to draft because I've just discovered that I can turn strict mode on and it's catching a lot more infractions in our current spec. Look into that now

@github-actions GitHub Actions
Copy link

API specs implemented for 307/649 (47%) APIs.

5 similar comments
@github-actions GitHub Actions
Copy link

API specs implemented for 307/649 (47%) APIs.

@github-actions GitHub Actions
Copy link

API specs implemented for 307/649 (47%) APIs.

@github-actions GitHub Actions
Copy link

API specs implemented for 307/649 (47%) APIs.

@github-actions GitHub Actions
Copy link

API specs implemented for 307/649 (47%) APIs.

@github-actions GitHub Actions
Copy link

API specs implemented for 307/649 (47%) APIs.

@nhtruong
Copy link
Collaborator Author

nhtruong commented May 14, 2024 •
edited
Loading

API specs implemented for 307/649 (47%) APIs.

@dblock These comments are getting out of hand 😅 There should be a cool down period, or maybe it should only be triggered when the PR is first created and/or ONLY run it constantly like this when a certain tag is attached to the PR?

@dblock
Copy link
Member

dblock commented May 14, 2024

API specs implemented for 307/649 (47%) APIs.

@dblock These comments are getting out of hand 😅 There should be a cool down period, or maybe it should only be triggered when the PR is first created and/or ONLY run it constantly like this when a certain tag is attached to the PR?

For sure. It could delete the previous comment/update the existing one, etc. Feel free to open an issue.

@nhtruong nhtruong marked this pull request as ready for review May 14, 2024 17:09
@github-actions GitHub Actions
Copy link

API specs implemented for 307/649 (47%) APIs.

This was referenced May 14, 2024
Closed
Merged
@nhtruong
Copy link
Collaborator Author

@dblock @Xtansia This is ready for review again

dblock
dblock reviewed May 16, 2024
View reviewed changes
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.

It's not a lot of changes, but you might want to split up logger changes.

tools/src/logger.ts Outdated Show resolved Hide resolved
tools/src/logger.ts Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

API specs implemented for 307/649 (47%) APIs.

1 similar comment
@github-actions GitHub Actions
Copy link

API specs implemented for 307/649 (47%) APIs.

@github-actions GitHub Actions
Copy link

API specs implemented for 307/649 (47%) APIs.

dblock
dblock previously approved these changes May 17, 2024
View reviewed changes
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.

Write tests for logger, flip order. Sorry to be annoying ;) Otherwise this PR LGTM.

tools/src/Logger.ts Outdated
export enum LogLevel {
info = 1,
warn = 2,
error = 3
Copy link
Member

Choose a reason for hiding this comment

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

In all the loggers the order is usually the other way around, you should flip these.

https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahhh Today I learned. Thanks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's been updated now. I've squashed all commits too. Feel free to merge it after approving.

@nhtruong
Validate all schemas defined in the spec as valid JSON schemas
747be0a 
- Updated default arg values for merge.ts
- Added Logger to silence merger tool's warnings
- Added SchemasValidator

Signed-off-by: Theo Truong <[email protected]>
@dblock dblock merged commit b253667 into opensearch-project:main May 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xtansia Xtansia Xtansia left review comments

@dblock dblock dblock approved these changes

@harshavamsi harshavamsi Awaiting requested review from harshavamsi harshavamsi is a code owner

@sachetalva sachetalva Awaiting requested review from sachetalva sachetalva is a code owner

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

@Tokesh Tokesh Awaiting requested review from Tokesh Tokesh is a code owner

@aabeshov aabeshov Awaiting requested review from aabeshov aabeshov is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nhtruong @dblock @Xtansia