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

Tech debt/substitute legacy request #119888

Merged

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Nov 29, 2021

Resolves #119867

This PR handles the remaining usages of LegacyRequest in core http by substituting LegacyRequest with Hapi.Request and removes the type from the http interface.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@TinaHeiligers TinaHeiligers added auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes technical debt Improvement of the software architecture and operational architecture v8.1.0 Feature:Legacy Removal Issues related to removing legacy Kibana labels Nov 29, 2021
@TinaHeiligers TinaHeiligers marked this pull request as ready for review November 29, 2021 21:27
@TinaHeiligers TinaHeiligers requested a review from a team as a code owner November 29, 2021 21:27
private authHeadersCache = new WeakMap<LegacyRequest, AuthHeaders>();
public set = (request: KibanaRequest | LegacyRequest, headers: AuthHeaders) => {
private authHeadersCache = new WeakMap<Request, AuthHeaders>();
public set = (request: KibanaRequest | Request, headers: AuthHeaders) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public set = (request: KibanaRequest | Request, headers: AuthHeaders) => {
public set = (request: KibanaRequest, headers: AuthHeaders) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I misunderstood the suggestion for replacing LegacyRequest with Hapi.Request as removing the type alias. Thanks for clarifying!

constructor(private readonly canBeAuthenticated: () => boolean) {}
public set = (request: KibanaRequest | LegacyRequest, state: unknown) => {
public set = (request: KibanaRequest | Request, state: unknown) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public set = (request: KibanaRequest | Request, state: unknown) => {
public set = (request: KibanaRequest, state: unknown) => {

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Nov 30, 2021

Choose a reason for hiding this comment

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

We still need to allow Hapi.Request as the request param type for configuring the auth scheme. Since this is private and internal to Core during the server setup/start, I think it's 'safe' enough to allow a Hapi.Request as well.

If we did want to revisit that, we'd need to do some more refactoring and confirm that changes would be ok with unauthorized Kibana access and other auth work-flows.

this.storage.set(ensureRawRequest(request), state);
};
public get = <T = unknown>(request: KibanaRequest | LegacyRequest) => {
public get = <T = unknown>(request: KibanaRequest | Request) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public get = <T = unknown>(request: KibanaRequest | Request) => {
public get = <T = unknown>(request: KibanaRequest) => {

@TinaHeiligers TinaHeiligers enabled auto-merge (squash) November 30, 2021 16:40
@TinaHeiligers TinaHeiligers force-pushed the tech-debt/substitute-LegacyRequest branch from dfe45e0 to 1a6eccb Compare November 30, 2021 18:35
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers merged commit 338fe1a into elastic:main Nov 30, 2021
@TinaHeiligers TinaHeiligers deleted the tech-debt/substitute-LegacyRequest branch November 30, 2021 23:11
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 30, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 119888

@TinaHeiligers TinaHeiligers removed the auto-backport Deprecated - use backport:version if exact versions are needed label Dec 1, 2021
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting chore Feature:Legacy Removal Issues related to removing legacy Kibana release_note:skip Skip the PR/issue when compiling release notes technical debt Improvement of the software architecture and operational architecture v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup usage of LegacyRequest
4 participants