Fix tasks namespace #520
Fix tasks namespace #520
Conversation
Changes AnalysisCommit SHA: dec5cc6 API ChangesSummary
ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10782874511/artifacts/1911697885 API Coverage
|
Xtansia
force-pushed
the
fix/tasks-namespace
branch
3 times, most recently
from
2d4ac35 to
608065b
Compare
Xtansia
requested review from
dblock,
harshavamsi,
sachetalva,
nhtruong,
VachaShah,
Tokesh and
aabeshov
as code owners
spec/schemas/_common.yaml
Outdated
| @@ -1927,6 +1908,8 @@ components: | |||
| - remote_cluster_client | |||
| - transform | |||
| - voting_only | |||
| x-deprecated-enums: | |||
There was a problem hiding this comment.
The problem with x-deprected-enums is that it doesn't express since when. Would this be possible?
- value: master
x-deprecated-since: '2.0'
There was a problem hiding this comment.
It's unfortunate that enum: only accepts a string list, so we can't properly dedupe the values and associate info directly. We could do something like:
NodeRole:
type: string
enum:
- client
- cluster_manager
- coordinating_only
- data
- data_cold
- data_content
- data_frozen
- data_hot
- data_warm
- ingest
- master
- ml
- remote_cluster_client
- transform
- voting_only
x-deprecated-enums:
master:
message: 'Please use inclusive language.'
since: '2.0'Or if we want to generalize adding any extensions against specific values:
NodeRole:
type: string
enum:
- client
- cluster_manager
- coordinating_only
- data
- data_cold
- data_content
- data_frozen
- data_hot
- data_warm
- ingest
- master
- ml
- remote_cluster_client
- transform
- voting_only
x-enum:
cluster_manager:
x-version-added: '2.0'
master:
x-deprecation-message: 'Please use inclusive language.'
x-version-deprecated: '2.0'Obviously would ideally need a validator/linter for it to ensure values match up etc.
There was a problem hiding this comment.
Don't we transform these parts of the spec in the merger and could have special treatment for enums?
Otherwise I think x-enum is a better and more extensible idea.
There was a problem hiding this comment.
The x-vendor-extensions are only applicable to OpenAPI properties, not JSON Schemas. Further more, I don't think this level of granularity is necessary. We can add a description property for the schema that says The use of "master" as a node role has been deprecated. Use "cluster_manager" instead
There was a problem hiding this comment.
I also think that this repo needs branches for major versions, as it will get more and more complex to keep track of all the diffs between major versions in the same spec.
There was a problem hiding this comment.
@nhtruong Are you saying this PR is good as is? I'm ok with it, feel free to hit approve.
There was a problem hiding this comment.
I mean instead of x-deprecated-enums, which is not a supported property of JSON-schema, we should replace it with a description to keep it simple as the extra resolution of info is not worth the complexity. If we start adding x-deprecated-enums, then there will be x-deprecated-properties and such. The server still supports these deprecated values, we just need a way to inform the reader that they are deprecated one way or the other, and I think having it in the description will suffice.
There was a problem hiding this comment.
@nhtruong The x- are allowed on all schema objects according to OAS 3.1 (because "OpenAPI properties" are just schemas): https://spec.openapis.org/oas/v3.1.0.html#fixed-fields-19
The main reasoning for this is that we annotate these values as @Deprecated in Java (or [Obsolete] in .NET), so just putting it in the description is unhelpful.
There was a problem hiding this comment.
I think we should do https://stackoverflow.com/a/78232576.
Signed-off-by: Thomas Farr <[email protected]>
Signed-off-by: Thomas Farr <[email protected]>
Xtansia
force-pushed
the
fix/tasks-namespace
branch
from
7247d09 to
6248619
Compare
Signed-off-by: Thomas Farr <[email protected]>
Signed-off-by: Thomas Farr <[email protected]>
Spec Test Coverage Analysis
|
| try { | ||
| this.ajv.addSchema(schema, key); | ||
| } catch (e) { | ||
| throw new Error(`Failed to add schema ${key}: \`${JSON.stringify(schema)}\``, { cause: e }) |
|
@nhtruong you good with the enum solution? |
Signed-off-by: Thomas Farr <[email protected]>
Description
Describe what this change achieves.
Issues Resolved
List any issues this PR will resolve, e.g. Closes [...].
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.