[Core][Deprecations] omit deprecationDetails if needed it in the reques#114399
Merged
XavierM merged 6 commits intoelastic:masterfrom Oct 11, 2021
Merged
[Core][Deprecations] omit deprecationDetails if needed it in the reques#114399XavierM merged 6 commits intoelastic:masterfrom
XavierM merged 6 commits intoelastic:masterfrom
Conversation
XavierM
added
Team:Core
Contributor
|
Pinging @elastic/kibana-core (Team:Core) |
XavierM
changed the title
pgayvallet
reviewed
Oct 11, 2021
Comment on lines
68
to
71
| body: JSON.stringify({ | ||
| ...body, | ||
| deprecationDetails: { domainId }, | ||
| ...(omitDeprecationDetails ? {} : { deprecationDetails: { domainId } }), | ||
| }), |
Contributor
There was a problem hiding this comment.
@Bamieh do you know why we're adding this in the first place? I suspect more endpoints may fail to validate because of this added parameter.
- Are we documenting this anywhere?
- Shouldn't we just remove this behavior? What is it supposed to be used for?
Contributor
There was a problem hiding this comment.
The resolve API endpoint has no way of telling which deprecation domain ID the user is attempting to resolve (via UA or a direct endpoint call) unless each deprecation explicitly passed the domainId in the correctiveAction.api.body.
Is this documented?
I do mention that we pass theapi.bodyand the context of the deprecation in the docs
The field is shown in the example route in the functional tests
Shouldn't we just remove this behavior? What is it supposed to be used for?
the domainId is the id of the plugin and it is automatically passed to each deprecation. I am passing it to the api body as well to avoid plugins requirinig to manually pass their domainId to the api if they need it.
Looking at the current depreaction routes across kibana I don't see this used yet so we can remove it completely. I am curious why not just add this object to the schema of your routes @XavierM rather than omitting it by adding extra logic.
Contributor
There was a problem hiding this comment.
The resolve API endpoint has no way of telling which deprecation domain ID the user is attempting to resolve
Unless I understand
kibana/src/core/public/deprecations/deprecations_client.ts
Lines 48 to 50 in 249c5fb
wrong, we're performing a call against the endpoint registered via DomainDeprecationDetails.correctiveActions.api. as all these info are registered by the deprecation owners, I would expect them to be self-sufficient to resolve the deprecation?
I am passing it to the api body as well to avoid plugins requirinig to manually pass their domainId to the api if they need it
Sorry, to which API? do automatic resolution endpoints need to call a deprecation API somehow after the resolution?
Contributor
There was a problem hiding this comment.
Here is an example how a deprecation is registered:
The domainId is not passed by the plugins. It is automatically attached to the registered deprecation based on which plugin is doing this registration.
The plugins register their own routes to correct the deprecation, and since i'm providing the domainId during deprecation registration, i thought i'd also pass it to the route as well.
The correctiveAction.api is called by the deprecations service DeprecationsClient which is used by the UA
Contributor
There was a problem hiding this comment.
I had a zoom call with @XavierM and this approach of adding a flag LGTM for now. They are not creating their own route for resolving the deprecation so it is not possible to just pass this provided context object and allowing it in the route schema.
I think it is fine to have this flag inside correctiveActions.api.omitContextFromBody. We can revisit this in 8.x (if needed) since we can change it on a more comfortable schedule and we'll have less custom routes to resolve deprecations.
Contributor
Author
|
@elasticmachine merge upstream |
1 task
Contributor
Author
|
@elasticmachine merge upstream |
Contributor
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/maps/documents_source/docvalue_fields·js.maps app documents source docvalue_fields should format date fields as epoch_millis when data driven styling is applied to a date fieldStandard OutStack TraceMetrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: cc @XavierM |
kibanamachine
added a commit
to kibanamachine/kibana
that referenced
this pull request
Oct 11, 2021
…es (elastic#114399) * omit deprecationDetails if needed it * review I * doc update Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Contributor
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In this PR #113172, the API call fails because the request payload contains an unexpected deprecationDetails field. It looks like the deprecations client is adding this automatically.
Checklist