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

[ide] Add IntelliJ desktop IDE support #6270

Closed
wants to merge 7 commits into from
Closed

Conversation

corneliusludmann
Copy link
Contributor

@corneliusludmann corneliusludmann commented Oct 18, 2021

Description

This PR adds support for Intellij desktop IDEs.

Feature Flag: To be able to merge this into main without rolling this out to all users on the next deployment, the settings are only visible when you are an admin user. We could also introduce a dedicated feature flag for this. However, I think hiding it behind the admin flag is sufficient enough.

We could think about splitting this change into smaller PRs. However, it seems to me that parts of this change are not really viable and it would not make sense to cut them out.

Werft Build Secret: Because we should not leak the download URLs of the IntelliJ backends, we store them in the K8s secret jetbrains-secrets in the werft namespace and consume them during the build. Once the download URLs are public, we can remove this and add the URLs to the repo directly.

To Do

The goal of this PR is to get a basic implementation merged into main and improve the implementation with smaller PRs. That means that most of the following to-do items should be moved to separate issues instead of fixing them in this PR.

We should focus on this PR to fix issues that would break something only.

  • Squash commits before merging!
  • Consider making the ws-manager-api ide_image filed composite as discussed here: Create and use jetbrains' workspace images #5642 (comment)
  • Think about it twice to make sure that we are backward compatible.
  • Unit/integration tests?
  • Heartbeat plugin seems not to work. When I close the browser, after 5 minutes the workspace times out even when I type in the IntelliJ editor. We need to debug this. @atduarte → handled here: Address Jetbrains backend plugin issues #6280
  • Use proper IDE icons in the settings.
  • The password to join via Code With Me is gitpod (hard-coded). How should we handle this?
  • Add Stop Workspace button?

Related Issue(s)

Fixes #5642, fixes #5641

How to test

Note: The preferences settings are hidden. Only users with admin rights see the desktop IDE settings. If you are not the first user of the preview env, you need to run something like $ leeway run components:make-admin.

https://clu-intellij-ide.staging.gitpod-dev.com/workspaces

  1. Go to SettingsPreferences and enable Use Desktop IDE:
    image

  2. Start a workspace. You should see the following:
    image

  3. Click on Open IntelliJ IDEA IDE and follow the instructions.

  4. Click on Open VSCode in Browser and Open VSCode should open as usual.

Release Notes

NONE

Depending on the rollout strategy, we would probably don't have this in the release notes yet but add this to the PR that removes the feature flag.

Meta

/no-cc

  • /werft with-clean-slate-deployment

@atduarte
Copy link
Contributor

Because we should not leak the download URLs of the IntelliJ backends

We now can use the public EAP releases and avoid the secrets, no?
https://download.jetbrains.com/idea/code-with-me/remote-dev/ideaIU-213.4958.tar.gz

Heartbeat plugin seems not to work. When I close the browser, after 5 minutes the workspace times out even when I type in the IntelliJ editor. We need to debug this

I will look into what might be going on

The password to join via Code With Me is gitpod (hard-coded). How should we handle this?

That's something we didn't give any thought to as the code with me links weren't supposed to be used. We could randomly generate or use the workspace id but would need to inform the user of that somehow. cc @loujaybee


We should make it explicit in the preferences that this requires IDEA Ultimate.

@iQQBot
Copy link
Contributor

iQQBot commented Oct 18, 2021

I am interested in this test, can I take it? @corneliusludmann

@corneliusludmann
Copy link
Contributor Author

We should make it explicit in the preferences that this requires IDEA Ultimate.

@atduarte What does that mean? When I run the bash command I get from the Code With Me page, I'm able to join the Gitpod workspace without any requirements. Has this been changed with the latest release?

image

@loujaybee
Copy link
Member

loujaybee commented Oct 18, 2021

We should make it explicit in the preferences that this requires IDEA Ultimate.

For me (without IntelliJ yet installed) it opens the "code with me EAP" guest application.

I think those requirements only apply to the host, and in this case Gitpod is the host?

image

It appears that all guests shouldn't require licenses / IDEA ultimate?

As a guest, you don’t even need a JetBrains IDE installed (source)

Disclaimer: pardon my ignorance as I wrap my head around the implementation 😁

@loujaybee
Copy link
Member

loujaybee commented Oct 18, 2021

I am interested in this test, can I take it? @corneliusludmann

Bear with us, it shouldn't be too long before we publish and send out any external communications on how you can join part of the beta / early release so you can experiment and give us some feedback 🙏

@iQQBot
Copy link
Contributor

iQQBot commented Oct 18, 2021

ok

@loujaybee
Copy link
Member

With regards to the code with me link and splash page (screenshot below) I wonder how easily it would be to bring that step within the UX of Gitpod? It seems that the generation of the curl script is doing OS detection + pulling the ID, so we could display that script directly on the workspace page to avoid the redirect. However, long term I think we'd probably want to bundle the logic into the companion app, but that's probably redundant if we eventually get rid of the Code With Me method.

Note: Definitely not required within this PR, but thought I'd mention here.

@loujaybee
Copy link
Member

Looks like this also fixes #5641?

@gtsiolis
Copy link
Contributor

gtsiolis commented Oct 22, 2021

/werft run

👍 started the job as gitpod-build-clu-intellij-ide.138

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from corneliusludmann and additionally assign csweichel, jankeromnes after the PR has been reviewed.
You can assign the PR to them by writing /assign @csweichel @jankeromnes in a comment when ready.

Associated issue: #5642

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #6270 (1398e92) into main (1d7c6ca) will increase coverage by 15.72%.
The diff coverage is n/a.

❗ Current head 1398e92 differs from pull request most recent head c931375. Consider uploading reports for the commit c931375 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6270       +/-   ##
===========================================
+ Coverage   19.04%   34.76%   +15.72%     
===========================================
  Files           2        4        +2     
  Lines         168      975      +807     
===========================================
+ Hits           32      339      +307     
- Misses        134      596      +462     
- Partials        2       40       +38     
Flag Coverage Δ
components-image-builder-mk3-app 34.76% <ø> (?)
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/local-app/pkg/auth/pkce.go
components/local-app/pkg/auth/auth.go
...image-builder-mk3/pkg/orchestrator/orchestrator.go 39.16% <0.00%> (ø)
...ents/image-builder-mk3/pkg/orchestrator/monitor.go 24.90% <0.00%> (ø)
components/image-builder/pkg/resolve/resolve.go 32.65% <0.00%> (ø)
...ents/image-builder-mk3/pkg/orchestrator/metrics.go 48.14% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d7c6ca...c931375. Read the comment docs.

@corneliusludmann
Copy link
Contributor Author

corneliusludmann commented Oct 22, 2021

Could we split this PR? It's unfortunate that it mashes so many changes into one. If it were more separate, we could test the individual pieces in isolation, and get a better overview of what's coming in.

I would love to. However, as mentioned above, I don't see a good way to split this into viable pieces. Let's have a look at what we have in this change:

  1. Changes in the API of a) registry-facade (4c8638c) and b) ws-manager (1398e92) for the desktop IDE image layers
    PR: Add desktop IDE image support in registry-facade and ws-manager #6364

    Details `registry-facade`
    • components/registry-facade-api/go/imagespec.pb.go
    • components/registry-facade-api/go/provider.pb.go
    • components/registry-facade-api/imagespec.proto
    • components/registry-facade/pkg/registry/layersource.go
    • components/registry-facade/pkg/registry/registry.go
    Details `ws-manager` (depends on the changes in `registry-facade`)
    • components/ws-manager-api/core.proto
    • components/ws-manager-api/go/core.pb.go
    • components/ws-manager-api/typescript/src/core_pb.d.ts
    • components/ws-manager-api/typescript/src/core_pb.js
    • components/ws-manager/pkg/manager/create.go
    • components/ws-manager/pkg/manager/status.go
  2. Changes in the supervisor backend that accepts the desktop IDE image value and starts the IDE process (60d510e)
    PR: [supervisor] Add desktop IDE backend support #6365

    • components/supervisor-api/go/status.pb.go
    • components/supervisor-api/java/.project
    • components/supervisor-api/java/src/main/java/io/gitpod/supervisor/api/Status.java
    • components/supervisor-api/status.proto
    • components/supervisor/pkg/supervisor/config.go
    • components/supervisor/pkg/supervisor/services.go
    • components/supervisor/pkg/supervisor/supervisor.go
    • components/supervisor/supervisor-config.json
  3. Changes in the supervisor frontend and the dashboard that shows a proper page when the workspace is ready (911541c)
    PR: [supervisor] Add desktop IDE frontend support #6366

    • components/dashboard/src/start/StartPage.tsx
    • components/dashboard/src/start/StartWorkspace.tsx
    • components/supervisor/frontend/src/ide/supervisor-service-client.ts
    • components/supervisor/frontend/src/index.ts
    • components/supervisor/frontend/src/shared/loading-frame.ts
  4. New JetBrains desktop IDE image (c6e8671)
    PR: [ide] Add JetBrains IDE images #6367

    • .werft/build.ts
    • .werft/build.yaml
    • chart/templates/blobserve-configmap.yaml
    • chart/templates/server-ide-configmap.yaml
    • chart/values.yaml
    • components/BUILD.yaml
    • components/ide/jetbrains/backend-plugin/BUILD.yaml
    • components/ide/jetbrains/image/*
  5. An option to choose the desktop IDE in the preferences of the dashboard (faa677c)
    PR: [server] Add preferences to choose a desktop IDE #6368

    • components/dashboard/src/settings/Preferences.tsx
    • components/dashboard/src/images/golandLogo.svg
    • components/dashboard/src/images/intellijIdeaLogo.svg
    • components/gitpod-protocol/src/protocol.ts
  6. Changes in server that pass the selected desktop IDE image to ws-manager (92e14cf)
    (requires changes 1 and 5 to compile)
    PR: [server] Pass selected desktop IDE to ws-manager #6369

    • components/gitpod-protocol/src/workspace-instance.ts
    • components/server/src/ide-config.ts
    • components/server/src/workspace/workspace-starter.ts

For each of the items in the list, I created a separate commit (see commit hashes in the list) as well as separate PRs (see links in the list). That makes sure that each piece compiles on its own and works deployed separately, but the ability to test the changes is pretty limited. I would suggest to keep this PR as an “umbrella” PR that shows the integration of all pieces and would update the commits here (to make sure the integration still works) when I do changes in the separate PRs due to reviewer feedback. Would this be a good compromise, @csweichel?

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Whoa! @corneliusludmann it felt ​really good to be able to browse all workspace files inside IntelliJ IDEA. 🔮

I kept running into some reconnection issues but maybe it was because of the pushed updates in #6270 (comment). ❗

Left some minor UX comments but feel free to skip them for now.

@@ -32,6 +33,8 @@ function getPhaseTitle(phase?: StartPhase, error?: StartWorkspaceError) {
return "Starting";
case StartPhase.Running:
return "Starting";
case StartPhase.IdeReady:
return "Your Workspace is Ready!";
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Wondering if a simpler phase would be better here.

Suggested change
return "Your Workspace is Ready!";
return "Running";

Copy link
Member

Choose a reason for hiding this comment

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

Added to: #6438

{desktopIdeFeatureEnabled &&
<div className="mt-4 space-x-4 flex">
<CheckBox
title="Use Desktop IDE"
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Shall we use the same verb as the action button on the loading page?

Suggested change
title="Use Desktop IDE"
title="Open in Desktop IDE"

Copy link
Member

Choose a reason for hiding this comment

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

Added to: #6438

</div>
<div className="mt-10 justify-center flex space-x-2">
<a target="_blank" href={this.state.desktopIde.link}><button>{this.state.desktopIde.label}</button></a>
<button className="secondary" onClick={() => window.parent.postMessage({ type: 'openBrowserIde' }, '*')}>Open VS Code in Browser</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

question: What do you think of rephrasing this and associating the browser IDE with the product?

Suggested change
<button className="secondary" onClick={() => window.parent.postMessage({ type: 'openBrowserIde' }, '*')}>Open VS Code in Browser</button>
<button className="secondary" onClick={() => window.parent.postMessage({ type: 'openBrowserIde' }, '*')}>Open in Gitpod</button>

Copy link
Contributor

Choose a reason for hiding this comment

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

Come across this, and I don't think it's a good idea. The "Open in Gitpod" buttons that exist around the internet in Git repositories will lead to whatever IDE you have configured in your account. This button "Open VS Code in Browser" will lead to VS Code no matter what.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point @atduarte!

@@ -0,0 +1,11 @@
{
"entrypoint": "/ide-desktop/startup.sh",
"entrypointArgs": [ "Open IntelliJ IDEA IDE" ],
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Does it matter if we mention the term IDE here?

Suggested change
"entrypointArgs": [ "Open IntelliJ IDEA IDE" ],
"entrypointArgs": [ "Open in IntelliJ IDEA" ],

</div>
<div className="mt-10 justify-center flex space-x-2">
<a target="_blank" href={this.state.desktopIde.link}><button>{this.state.desktopIde.label}</button></a>
<button className="secondary" onClick={() => window.parent.postMessage({ type: 'openBrowserIde' }, '*')}>Open VS Code in Browser</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: What do you think of changing the order of the actions so that the primary action is always on the outer right? This will follow the existing pattern that exists for timed out workspaces, and more.

Desktop IDE options Stopped Workspace
Screenshot 2021-10-22 at 7 29 37 PM Screenshot 2021-10-22 at 7 33 16 PM

Copy link
Member

Choose a reason for hiding this comment

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

Added to: #6438

<div className="mt-4 space-x-4 flex">
<CheckBox
title="Use Desktop IDE"
desc="Choose whether you would like to open your workspace in a desktop IDE instead."
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Given that we could add more local IDEs here in the future, could it help to make the UI less verbose by using a dropdown selection here instead of the card selection component? This could also help with using proper identation of the options.

Early design draft with a dropdown selection
Screenshot 2021-10-22 at 7 34 04 PM

Copy link
Member

Choose a reason for hiding this comment

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

There's a longer conversation regarding this one in: #5641 (comment), let me cross-reference it there, also.

<a target="_parent" href={this.state.workspace?.contextURL}><p className="w-56 truncate hover:text-blue-600 dark:hover:text-blue-400" >{this.state.workspace?.contextURL}</p></a>
</div>
</div>
<div className="mt-10 justify-center flex space-x-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Although out of the scope of these changes, here's an early draft on how these actions could be unified, compressed, and always available during workspace loading phase based on a relevant discussion (internal). 🍔

We could later also include a dropdown option in the workspace start so that users can change this setting ad-hoc during workspace start in the loading phases where this is possible. 💡

In contrast, a dropdown selection could allow the loading phase to proceed and only when ready and needed would prompt to open the local VS Code application, but I could be wrong here. This would also connect and surface the selection option from /settings.

Thinking out loud here, I’d expect this option to be non-editable once the loading phase is near the end where we load the Web IDE.

Screenshot 2021-10-22 at 7 43 50 PM

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the heads up!

Let's address this in: #5641

@csweichel
Copy link
Contributor

csweichel commented Oct 22, 2021

For each of the items in the list, I created a separate commit (see commit hashes in the list) as well as separate PRs (see links in the list). That makes sure that each piece compiles on its own and works deployed separately, but the ability to test the changes is pretty limited. I would suggest to keep this PR as an “umbrella” PR that shows the integration of all pieces and would update the commits here (to make sure the integration still works) when I do changes in the separate PRs due to reviewer feedback. Would this be a good compromise, @csweichel?

That's awesome! Thank you very much for going this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
9 participants