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

Added support for testing multiple distributions. #483

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dblock
Copy link
Member

@dblock dblock commented Aug 8, 2024

Description

Introduces x-distributions that allows labelling APIs for various distributions and services. I propose we use opensearch.org for the default distribution, aos and aoss for the two Amazon managed services, and others can add their own.

If an API is not present in all distributions one has to list the distributions it is present in. I think it's better than having a negative list so that we can claim support for a well-known list of distributions in the spec and not have piecemeal support.

With this change, npm run test:spec -- --opensearch-distribution=amazon-managed passes against AOS 2.13.

Corrects semver checks for ranges with multiple values (e.g. >= 2.0, < 3).

Issues Resolved

Part of #84.
Closes #475.

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

github-actions bot commented Aug 8, 2024

Changes Analysis

Commit SHA: 101ec85
Comparing To SHA: 9d3bc34

API Changes

Summary

├─┬Paths
│ └─┬/
│   └─┬GET
│     └─┬Extensions
│       └──[➕] x-distributions-excluded (13:9)
└─┬Components
  ├──[➖] schemas (46999:7)❌ 
  ├──[➕] schemas (47015:7)
  ├─┬nodes._common:ShardIndexingPressureStats
  │ ├──[➕] properties (46749:9)
  │ └─┬total_rejections_breakup_shadow_mode
  │   └──[🔀] $ref (47015:13)❌ 
  ├─┬nodes._common:IoStatDevice
  │ ├──[➕] properties (46346:9)
  │ ├──[➕] properties (46344:9)
  │ ├──[➕] properties (46340:9)
  │ └──[➕] properties (46342:9)
  ├─┬nodes.info:NodeJvmInfo
  │ ├──[➖] required (47694:11)❌ 
  │ ├──[➖] required (47689:11)❌ 
  │ ├──[➖] required (47695:11)❌ 
  │ ├──[➖] required (47686:11)❌ 
  │ ├──[➖] required (47687:11)❌ 
  │ ├──[➖] required (47692:11)❌ 
  │ ├──[➖] required (47693:11)❌ 
  │ └─┬using_bundled_jdk
  │   └──[🔀] type (47688:13)❌ 
  ├─┬nodes._common:ShardSearchBackpressureTaskCancellationStats
  │ ├──[➕] properties (46797:9)
  │ └──[➕] properties (46799:9)
  ├─┬nodes.info:NodeOperatingSystemInfo
  │ ├──[➖] required (47728:11)❌ 
  │ ├──[➖] required (47723:11)❌ 
  │ ├──[➖] required (47725:11)❌ 
  │ └──[➖] required (47726:11)❌ 
  └─┬nodes.info:NodeInfo
    ├──[➖] required (47166:11)❌ 
    ├──[➖] required (47162:11)❌ 
    ├──[➖] required (47165:11)❌ 
    └──[➖] required (47169:11)❌ 

Document Element Total Changes Breaking Changes
paths 1 0
components 26 18
  • BREAKING Changes: 18 out of 27
  • Modifications: 2
  • Removals: 16
  • Additions: 9
  • Breaking Removals: 16
  • Breaking Modifications: 2

Report

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

API Coverage

Before After Δ
Covered (%) 524 (51.32 %) 524 (51.32 %) 0 (0 %)
Uncovered (%) 497 (48.68 %) 497 (48.68 %) 0 (0 %)
Unknown 26 26 0

Copy link

github-actions bot commented Aug 8, 2024

Spec Test Coverage Analysis

Total Tested
550 213 (38.73 %)

@nhtruong
Copy link
Collaborator

nhtruong commented Aug 8, 2024

Should we have opensearch.org as the default distribution for the test stories as well since it's the default in the spec?

@@ -146,6 +146,7 @@ This repository includes several OpenAPI Specification Extensions to fill in any
- `x-ignorable`: Denotes that the operation should be ignored by the client generator. This is used in operation groups where some operations have been replaced by newer ones, but we still keep them in the specs because the server still supports them.
- `x-global`: Denotes that the parameter is a global parameter that is included in every operation. These parameters are listed in the [spec/_global_parameters.yaml](spec/_global_parameters.yaml).
- `x-default`: Contains the default value of a parameter. This is often used to override the default value specified in the schema, or to avoid accidentally changing the default value when updating a shared schema.
- `x-distributions`: Contains a list of distributions known to include the API. Use `opensearch.org` for the official distribution, `aos` for Amazon Managed OpenSearch, and `aoss` for Amazon OpenSearch Serverless.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think having aos and aoss as shortened versions is not immediately grokkable to someone without some AWS specific context. Even as someone with that context having one be a prefix of the other immediately hurts readability. Could we expand these to amazon-managed & amazon-serverless given we have a full form opensearch.org?

We should think ahead for what precedent we set if in future we end up with contributions related to things like Oracle Cloud Infrastructure Search with OpenSearch. (oci, ocisos, oracle, oracle-managed, oci-managed, etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. One thought I had is to use URIs, e.g. aws.amazon.com/opensearch-service? WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be a bit verbose, AOSS would be aws.amazon.com/opensearch-service/features/serverless presumably.

Either way we'd likely need to define an enum somewhere and validate correctness, as spelling mistakes will be a PITA

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to use ajv.addKeyword to define a custom type for the new x-distribution options, but couldn't make it work. Maybe @nhtruong can help me make these values check against an enum on top of this PR.

Copy link
Collaborator

@nhtruong nhtruong Aug 9, 2024

Choose a reason for hiding this comment

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

We're only using AJV to validate JSON Schemas in the spec. x-distribution is not used in a schema, but rather an operation. So it should be validated similar to how we validate x-operation-group and other extensions.

In the very near future, we should create a json-schema for the namespace files and another for the category files in the ./spec folder. Even though we currently only require our yaml files to be OpenAPI valid, we actually require a much stricter json-schema for these files:

  • x-operation-group is supported but a typo like x-opperation-group should not.
  • Someone uses a new and valid OpenAPI keyword that the clients should handle accordingly but it got through the PR review without updating the Client Gen doc.
  • #paths is not allowed in category yaml files
  • #components/schemas is not allowed in namespace yaml files
  • We can enforce validation on the extensions for free through AJV.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Xtansia
Copy link
Collaborator

Xtansia commented Aug 8, 2024

Should we have opensearch.org as the default distribution for the test stories as well since it's the default in the spec?

I feel like we might really want both an "allow"/"include" and "deny"/"exclude" variant. As in the majority of cases all or most distributions will support an API and just aoss doesn't. So my thought is you'd generally by default want new tests/operations to be automatically opted into for all/new distributions and then whittled down as necessary. (Assuming we get automated testing against the managed distributions)

@dblock
Copy link
Member Author

dblock commented Aug 9, 2024

I've added include and exclude variants, etc., but no value validation in this PR. Please take another look?

We'll need jamesmbourne/aws4-axios#1621 to make serverless work.

@dblock
Copy link
Member Author

dblock commented Aug 13, 2024

Should we have opensearch.org as the default distribution for the test stories as well since it's the default in the spec?

I feel like we might really want both an "allow"/"include" and "deny"/"exclude" variant. As in the majority of cases all or most distributions will support an API and just aoss doesn't. So my thought is you'd generally by default want new tests/operations to be automatically opted into for all/new distributions and then whittled down as necessary. (Assuming we get automated testing against the managed distributions)

On this note. Do you @nhtruong @Xtansia have a (strong) preference for:

  1. x-distributions-included, x-distributions-included and distributions: in tests (current state of the PR).
  2. ...
    distributions:
       included:
       excluded: 
    
    in tests.
  3. ...
    distributions-included:
    distributions-excluded: 
    
    in tests.

DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
tools/src/tester/StoryEvaluator.ts Outdated Show resolved Hide resolved
tools/src/tester/TestRunner.ts Outdated Show resolved Hide resolved
tools/src/types.ts Outdated Show resolved Hide resolved
tools/src/OpenSearchHttpClient.ts Outdated Show resolved Hide resolved
@Xtansia
Copy link
Collaborator

Xtansia commented Aug 13, 2024

I think ideally we'd want distributions: {included: [], excluded: []} in tests, could still have distributions: ['foo'] that just shortcuts to distributions: {included: ['foo']}.

There might be better verbiage than (in|ex)cluded (something like supported maybe?) but I haven't come up with any other options I'm 100% happy with so not critical to this PR.

Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
@dblock dblock force-pushed the add-distributions branch 2 times, most recently from 4dab48b to 2ef9985 Compare August 13, 2024 23:38
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
@dblock
Copy link
Member Author

dblock commented Aug 13, 2024

I think ideally we'd want distributions: {included: [], excluded: []} in tests, could still have distributions: ['foo'] that just shortcuts to distributions: {included: ['foo']}.

There might be better verbiage than (in|ex)cluded (something like supported maybe?) but I haven't come up with any other options I'm 100% happy with so not critical to this PR.

Made updates per comments and rebased. We can expand into includes and excludes for the distribution in a future PR.

@@ -64,6 +64,7 @@ export interface AwsAuth {

export interface OpenSearchHttpClientOptions {
url?: string
distribution?: string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also don't need these other changes here now as it's not actually used by OpenSearchHttpClient

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.

[BUG] NodeInfo required fields can be null in some instances
3 participants