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

Do not use outdated request package in tests. #109225

Merged

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented Aug 19, 2021

Summary

In this PR we switch to a well-maintained tough-cookie package to parse set-cookie HTTP headers in our tests instead of relying on the deprecated and unmaintained request package. Unlike request we make tough-cookie a dev-only dependency.

Related to: #109202

@azasypkin azasypkin added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.16.0 labels Aug 19, 2021
@azasypkin azasypkin force-pushed the issue-xxx-ditch-request-from-tests branch from 27226a2 to 1e89503 Compare August 23, 2021 14:51
@@ -103,7 +103,7 @@ interface Storage {
}

function retrieveSessionCookie(cookies: string) {
const sessionCookie = request.cookie(cookies);
const sessionCookie = parseCookie(cookies);
Copy link
Member Author

Choose a reason for hiding this comment

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

note: unfortunately couldn't test this change as the entire test suite is skipped for the last 5 months 😢

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

@azasypkin azasypkin marked this pull request as ready for review August 24, 2021 05:55
@azasypkin azasypkin requested review from a team as code owners August 24, 2021 05:55
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

After reading "cookie" a couple hundred times, I realized what a weird word it is!

LGTM, thanks for taking care of this 💪

// Do not assert on the `authentication_realm`, as the value differes for on-prem vs cloud
// Do not assert on the `authentication_realm`, as the value differs for on-prem vs cloud
Copy link
Contributor

Choose a reason for hiding this comment

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

😄

@azasypkin azasypkin merged commit d37b365 into elastic:master Aug 24, 2021
@azasypkin azasypkin deleted the issue-xxx-ditch-request-from-tests branch August 24, 2021 14:08
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 26, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 109225 or prevent reminders by adding the backport:skip label.

@azasypkin azasypkin added auto-backport Deprecated - use backport:version if exact versions are needed and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Aug 26, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants