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

Add additionalProperties to ErrorCause and fix creation_date type for list_dangling_indices #462

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

Xtansia
Copy link
Collaborator

@Xtansia Xtansia commented Aug 2, 2024

Description

Add additionalProperties to ErrorCause and fix creation_date type for list_dangling_indices

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 Aug 2, 2024

Changes Analysis

Commit SHA: 6ac6529
Comparing To SHA: 59a7ff4

API Changes

Summary

└─┬Components
  ├─┬dangling_indices.list_dangling_indices:DanglingIndex
  │ └──[➕] properties (41985:9)
  ├─┬_common:UnitMillis
  │ └──[➕] format (28375:15)❌ 
  └─┬_common:ErrorCause
    └──[➕] additionalProperties (26726:9)❌ 

Document Element Total Changes Breaking Changes
components 3 2
  • BREAKING Changes: 2 out of 3
  • Additions: 3
  • Breaking Additions: 2

Report

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

API Coverage

Before After Δ
Covered (%) 510 (49.95 %) 510 (49.95 %) 0 (0 %)
Uncovered (%) 511 (50.05 %) 511 (50.05 %) 0 (0 %)
Unknown 24 24 0

@Xtansia Xtansia force-pushed the spec-fix/dangling_indices branch from 08b0477 to e94e248 Compare August 2, 2024 02:53
Copy link
Contributor

github-actions bot commented Aug 2, 2024

Spec Test Coverage Analysis

Total Tested
534 118 (22.1 %)

@Xtansia Xtansia force-pushed the spec-fix/dangling_indices branch from e94e248 to 6bffec2 Compare August 2, 2024 02:56
@Xtansia Xtansia marked this pull request as ready for review August 2, 2024 02:56
@@ -53,7 +53,7 @@ export function determine_possible_schema_types (doc: OpenAPIV3.Document, schema
if (schema?.anyOf !== undefined) return collect_all(schema.anyOf)
if (schema?.oneOf !== undefined) return collect_all(schema.oneOf)

if (schema == null || Object.keys(schema).filter(k => k !== 'description').length == 0) return SCHEMA_OBJECT_TYPES
if (schema == null || Object.keys(schema).filter(k => k !== 'description' && k !== 'title').length == 0) return SCHEMA_OBJECT_TYPES
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If nothing is specified in the schema other than "informational" properties (ie. description or title) than it can be any type.

spec/schemas/_common.yaml Outdated Show resolved Hide resolved
… list_dangling_indices

Signed-off-by: Thomas Farr <tsfarr@amazon.com>
@Xtansia Xtansia force-pushed the spec-fix/dangling_indices branch from 13778fb to 6ac6529 Compare August 5, 2024 05:06
Comment on lines +16 to +17
creation_date:
$ref: '_common.yaml#/components/schemas/DateTime'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm thinking we might want some way of annotating properties as being the human readable variant of another property and dependent on ?human. Would be useful for documentation or want to do some custom logic in clients/generators.

Maybe something like:

creation_date:
  x-human-readable-of: creation_date_millis

Thoughts @dblock @nhtruong?

Copy link
Member

Choose a reason for hiding this comment

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

It does feel a little too special, applying only to the _cat API. Are there exceptions where something like x always have a pair of x_millis and lives only under _cat?

Copy link
Collaborator Author

@Xtansia Xtansia Aug 5, 2024

Choose a reason for hiding this comment

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

Well it's not only the _cat api as this particular one is in the dangling_indices api and not only applies to _millis variants, also bytes ones

https://github.com/search?q=repo%3Aopensearch-project%2FOpenSearch%20humanReadableField&type=code

Copy link
Member

Choose a reason for hiding this comment

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

Would it be over-engineering to hide it under a type?

total:
   type: ...HumanlyReadableByteSize
   description: Total number of bytes.

would get automatically expanded as total and total_in_bytes when spec gets generated?

I'm just thinking out loud. The drawback of x-human-readable-of is that it's a relationship that needs to be validated. If you think it's the cleanest way to do it, go for it - and maybe @nhtruong has some other ideas?

@dblock dblock merged commit 1061972 into opensearch-project:main Aug 5, 2024
15 checks passed
@Xtansia Xtansia deleted the spec-fix/dangling_indices branch August 5, 2024 20:42
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.

2 participants