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

Deprecation process for the rest spec #38102

Closed
Mpdreamz opened this issue Jan 31, 2019 · 5 comments
Closed

Deprecation process for the rest spec #38102

Mpdreamz opened this issue Jan 31, 2019 · 5 comments
Labels
:Core/Infra/REST API REST infrastructure and utilities team-discuss

Comments

@Mpdreamz
Copy link
Member

A couple of breaking changes went into the 6.6.0 branch on the rest-api-spec folder:

  • ccr.resume_follow.json changed body from required to false.
  • Deprecate _source_include and _source_exclude url parameters #33475 changed _source_include to _source_includes
    • in the case of get.json it added _source_includes next to _source_include
    • in all other places it replaced the old notation with the new.
    • @elastic/es-clients was pinged on this ticket 👍 but we did not pick it up 😢

Opening this issue to serve as a reminder that changes in rest-api-spec should be additive only.

The other is to discuss how we should denote deprecation more structurally

e.g in indices.clear_cache.json

"field_data": {
    "type" : "boolean",
    "description" : "Clear field data. This is deprecated. Prefer `fielddata`."
}, 

suggestion:

"field_data": {
    "type" : "boolean",
    "description" : "Clear field data.",
    "deprecated": "fielddata"
}, 

Where an "" empty string denotes a removed but still supported param.

This would allow us to more clearly support an deprecation such as made #33475 as additive change in the rest spec.

In 7.0 we will also have deprecated paths.

In some cases the deprecated urls are removed completely from the spec, which is more clear cut since it basically only drops a prefix (_xpack in e.g the machinelearning API's). Removing is totally the right thing to do here.

In others index.json

"/{index}/{type}/{id}" is deprecated but still listed.

I think clients should keep generating this url to support users migrating but the spec needs to be able to signal its a deprecated API.

    "url": {
      "path": "/{index}/_doc",
      "paths": ["/{index}/{type}", "/{index}/_doc/{id}", "/{index}/_doc"],
      "deprecated_paths": ["/{index}/{type}/{id}"]
@jasontedor
Copy link
Member

ccr.resume_follow.json changed body from required to false.

CCR is in beta in 6.5 and subject to breakage in a minor version.

@jasontedor jasontedor added :Core/Infra/REST API REST infrastructure and utilities team-discuss labels Jan 31, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@codebrain
Copy link
Contributor

codebrain commented Feb 1, 2019

100% agree that developers should be allowed to change features, especially for those in their infancy.

However, the current specification does not sufficiently document that a feature is; experimental, beta, production (any others?). An adoption of such an attribute in the specification would enable the client developers to set better expectations to users when they are using a feature. In C# for example we have mechanisms in the language itself for communicating this clearly in the intellisense system. It may even award us some grace when handling the impact of accidentally missing such changes. We could even generate our own "stop the line" signalling in the build process to ensure such changes are manually reviewed.

Last count we are at 235 REST API specification documents. Can I please table the idea that such changes are best communicated through the specification itself, (perhaps with the adoption of an attribute?), as opposed to an out-of-band mechanism?

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Feb 5, 2019

Opened: #38413 to address your last point @codebrain.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Feb 5, 2019

Closing this in favor of: #35262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities team-discuss
Projects
None yet
Development

No branches or pull requests

4 participants