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

feat: [M3-8332] - Use new "lish" API instead of "lish_token" #10656

Merged
merged 5 commits into from
Jul 11, 2024

Conversation

plisiecki1
Copy link
Contributor

Description 📝

This change upgrades the backend weblish/glish session creation requests from the older lish_token API to a newer lish API. This newer API includes support for both core and distributed regions.

Changes 🔄

List any change relevant to the reviewer.

  • This change uses the new lish API which returns the full URLs rather than leaving it up to cloud manager to compute. For core regions, this should result in the same URL as cloud manager would have computed.
  • The new lish API also returns values to use as WebSocket "protocols"; this will currently be an empty list for core regions, but include session information for distributed regions.

Target release date 🗓️

ASAP

Preview 📷

There are no user-visible changes.

How to test 🧪

Create weblish and glish sessions against a host. It should work exactly as before for core regions. In the browser Developer Tools, you should see accesses to the lish endpoint and not the lish_token endpoint.

Prerequisites

No special setup is required.

Reproduction steps

N/A

Verification steps

See "How to test" above.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

Commit message and pull request title format standards

Note: Remove this section before opening the pull request
Make sure your PR title and commit message on squash and merge are as shown below

<commit type>: [JIRA-ticket-number] - <description>

Commit Types:

  • feat: New feature for the user (not a part of the code, or ci, ...).
  • fix: Bugfix for the user (not a fix to build something, ...).
  • change: Modifying an existing visual UI instance. Such as a component or a feature.
  • refactor: Restructuring existing code without changing its external behavior or visual UI. Typically to improve readability, maintainability, and performance.
  • test: New tests or changes to existing tests. Does not change the production code.
  • upcoming: A new feature that is in progress, not visible to users yet, and usually behind a feature flag.

Example: feat: [M3-1234] - Allow user to view their login history


@plisiecki1 plisiecki1 marked this pull request as ready for review July 8, 2024 21:33
@plisiecki1 plisiecki1 requested a review from a team as a code owner July 8, 2024 21:33
@plisiecki1 plisiecki1 requested review from mjac0bs and hana-akamai and removed request for a team July 8, 2024 21:33
Copy link

github-actions bot commented Jul 9, 2024

Coverage Report:
Base Coverage: 82.4%
Current Coverage: 82.4%

@hana-akamai hana-akamai added the Gecko Beta Relating to Gecko project label Jul 9, 2024
Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Use new lish API ✅
Core regions load lish/glish the same as prod ✅
New endpoint returns WebSocket protocols for distributed regions ✅
WebSocket protocols are empty for core regions ✅

Is it expected that the lish console is blank for distributed regions? Granted it doesn't work for me in prod either (just displays a loading spinner)

Distributed region Core region
Screenshot 2024-07-09 at 12 37 19 PM Screenshot 2024-07-09 at 12 38 13 PM

packages/api-v4/src/linodes/linodes.ts Outdated Show resolved Hide resolved
packages/manager/src/features/Lish/Glish.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Lish/Lish.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Lish/Lish.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Lish/Lish.tsx Outdated Show resolved Hide resolved
packages/manager/src/queries/linodes/linodes.ts Outdated Show resolved Hide resolved
* Remove ticket numbers.
* Add `LinodeLishData` interface to pass around the result of the `lish` API.
* Address eslint formatting issue.
@plisiecki1
Copy link
Contributor Author

It is expected that "distributed" will just spin because the backend configuration is not yet 100% complete.

@hana-akamai
Copy link
Contributor

@plisiecki1 It seems like lish requests are being spammed for the distributed regions on this branch

Screen.Recording.2024-07-10.at.12.13.29.PM.mov

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Observed the lish network request as expected in dev tools for weblish and glish, with empty protocols for core regions, and session protocol for a distributed region.

Aside from Hana's last observation, feedback addressed + Hana's changes looked good to me.

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Jul 10, 2024
@plisiecki1
Copy link
Contributor Author

@mjac0bs @bnussman-akamai @hana-linode - I have attempted to address the "spamming" of requests by improving the error/retry logic so that we can retry only several times within any short period of time. Is this too much to take in this change? We could go with the previously reviewed version and then take on the other improvements later on if that seems like a better approach.

@hana-akamai
Copy link
Contributor

@plisiecki1 These are a lot of new changes, let's go back to the previous version and merge that, then open a follow up PR for improvements

@plisiecki1
Copy link
Contributor Author

I just force pushed to revert the additional improvements, which we can work on in a separate follow-up PR. The code is now exactly the same commit which was previously reviewed.

@hana-akamai hana-akamai merged commit 91d0e03 into linode:develop Jul 11, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Gecko Beta Relating to Gecko project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants