[API Deprecations] Skip HTTP resources #207725
Conversation
afharo
added
release_note:skip
|
Pinging @elastic/kibana-core (Team:Core) |
There was a problem hiding this comment.
Did not test locally, but changes make sense to me!
The only part that I'm not quite following in this description is:
they pass the current filters because they are not categorized as public
When we register HTTP resources we categorize them explicitly as public to avoid triggering the "internal" access deprecation (and bc they are a special version of "public"). But maybe there is a code path for some of these registrations that does not preserve that?
|
Also, in our testing, I do not recall seeing any HTTP resource deprecations being surfaced 🤔 , I expect they are now appearing in Upgrade Assistant (which definitely is a bug). |
There was a problem hiding this comment.
I am fine with this change. Although it might be better to tailor this during logging rather than tinkering with the getIsAccessApiDeprecation conditions.
Feel free to merge this PR however I think its better to add this condition in an if statement wrapping the logging instead of changing the getIsAccessApiDeprecation. Having this extra if statement is leading to the same behavior in the deprecations. But from a code maintenance POV its redundant code that might restrict our implementation in the future if we happen to want to change things for access api deprecation.
I expect they are now appearing in Upgrade Assistant (which definitely is a bug).
No they do not appear in the UA because the “public” route definition omits them. i did test for that as well when i was working in this area
Code here https://github.com/elastic/kibana/blob/main/src/core/packages/http/router-server-internal/src/route.ts#L167-L175
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]Public APIs missing comments
cc @afharo |
| function getPostValidateEventMetadata( | ||
| request: AnyKibanaRequest, | ||
| routeInfo: RouteInfo | ||
| ): PostValidationMetadata { | ||
| return { | ||
| deprecated: routeInfo.deprecated, | ||
| isInternalApiRequest: request.isInternalApiRequest, | ||
| isPublicAccess: isPublicAccessApiRoute(routeInfo), |
There was a problem hiding this comment.
| isPublicAccess: isPublicAccessApiRoute(routeInfo), | |
| isPublicAccess: routeInfo.access === 'public' |
On alternative to the proposed changes is to preserve the previous behaviour instead of the using/fixing isPublicAccessApiRoute here.
|
Superseded by #207829. Closing |
Summary
Follow up to #199026
Related to #199616
When working on #199616, we noticed that we counted (and now logged) all the APIs serving http-resources (like the /bundles/ routes).
This PR improves our filter to avoid counting and logging those ones (they pass the current filters because they are not categorized as public, and they are technically not called internally (the browser loads them without the internal header).