search

gitpod-io / gitpod

The developer platform for on-demand cloud development environments to create software faster and more securely.
https://www.gitpod.io
GNU Affero General Public License v3.0
13.02k stars 1.25k forks source link

[code] fix gp cli while running next to jb product #8191

Open akosyakov opened 2 years ago

akosyakov commented 2 years ago

Since https://github.com/gitpod-io/gitpod/pull/8025 gp open/preview always exepct JB product if it is enabled, so they break in VS Code Web. We should redesign it by relying on notification service on supervisor instead of env vars.

andreafalzetti commented 2 years ago

after a chat with @akosyakov we concluded that we could start working on this feature incrementally, by adding initially only the endpoint in supervisor and testing it internally by supervisor CLI.

Then, we could have a follow-up ticket to integrate with gp-cli and so on.

andreafalzetti commented 2 years ago

@mustard-mh and I started working on this, we pair programmed for a couple of hours today and made some progress. Besides adding two new endpoints to supervisor-api we have discussed the integration with gp cli.

We've considered 2 approaches, when users use gp open or gp preview: A. gp cli calls supervisor which is now aware of the latest active IDE/client and supervisor executes the IDE's binary directly. B. gp cli calls supervisor to retrieve the latest active IDE/client, and gp cli executes the IDE's binary.

The biggest downside of approach A is that we need to implement in supervisor 2 endpoints, one for open one for preview mapping all parameters and flags of those 2 commands. For example gp open has a --wait flag which we should make sure to pass to supervisor. We would stricly couple gp-cli commands and supevisor API.

Another small downside for approach A, is that potentially makes harder returning any error to the user. If the open and preview logic is executed from supervisor, we would need to pro-actively make sure we can report back to the user, in case of failure. Instead, if the logic lives in gp cli the error would be returned directly to the user.

mustard-mh commented 2 years ago

What if we do not use <ide_identity> like vscode jetbrains_gateway etc enum, just use <editor_cli_location> like /ide-desktop/bin/idea-cli so that custom editor can be used here (eg. Users open workspaces without choose any IDE, but use vim with ssh)

And provide cmd like gp set-editor <editor_cli_location>

And we should also provide How to Use Custom IDE (via ssh) section in our doc and https://github.com/gitpod-io/gitpod/issues/6707 https://github.com/gitpod-io/gitpod/issues/7570

akosyakov commented 2 years ago

I think the upside of A that we have clean interface and hide implementation details of working with concrete IDEs behind them instead of relying on other ways. B does not sound so clean, since there will be one explicit protocol via interface and another implicit via binaries.

akosyakov commented 2 years ago

The biggest downside of approach A is that we need to implement in supervisor 2 endpoints, one for open one for preview mapping all parameters and flags of those 2 commands.

We should not do it. We should only map that is required for gp cli, i.e. open request should accept absolute path and preview should accept URL. That's it. If someone wants to use a specific capabilities of code CLI, then they should use it directly.

mustard-mh commented 2 years ago

If we exec in supervisor we cannot access to env var of current terminal session

exec in gp-cli it will behavior like user run it directly in this terminal

cc @akosyakov

akosyakov commented 2 years ago

If we exec in supervisor we cannot access to env var of current terminal session

Why do we need to access them? gp cli should resolve the absolute path and url and pass them to JB backend or VS Code windows via supervisor, neither of them are interested in env var of current terminal session.

mustard-mh commented 2 years ago

We don't need to access them currently😂

mustard-mh commented 2 years ago

If we only pass file_loc and url to supervisor, it means that we cannot set GIT_EDITOR=gp open --wait, git cmd like rebase will not be available if we switch IDE

--wait is available for both code and JetBrains IDE

So if we decide to exec in supervisor we can choose

  1. parse all args to supervisor as strings..
  2. parse all args as struct if needed
  3. forget about GIT_EDITOR

cc @akosyakov

akosyakov commented 2 years ago

Right, for open request we should also have wait property.

Are you worrying about backward compatibility to code? gp open does not work like an alias for code or idea-cli but rathe is using them right now as an implementation details. If someone abusing it they should rather to switch using code or idea-cli directly. It will express assumptions more clearly.

mustard-mh commented 2 years ago

I still prefer exec in gp cli, run in supervisor means we need to provide a func like func Exec(argv0 string, argv []string, envv []string) error

open need --wait preview need --preview

$ gitpod /workspace $ env | grep GP_PREVIEW_BROWSER
GP_PREVIEW_BROWSER=/ide/bin/remote-cli/gitpod-code --preview

So why not ask supervisor for what ide user using now (or the cli location https://github.com/gitpod-io/gitpod/issues/8191#issuecomment-1096110343), and exec the cli in gp cli

we also parse env before https://github.com/gitpod-io/gitpod/blob/96199858ad66349aa24db7dea3fba8d31fcc606a/components/gitpod-cli/cmd/preview.go#L63-L66 https://github.com/gitpod-io/gitpod/blob/c8526aeff02a354c1b0a5845bde07b36fa455ae6/components/gitpod-cli/cmd/open.go#L57-L60

akosyakov commented 2 years ago

I still prefer exec in gp cli, run in supervisor means we need to provide a func like func Exec(argv0 string, argv []string, envv []string) error

We should not run anything in supervisor. It works following:

There is no anymore env vars or binaries involved, everything is done via clearly defined protocol.

mustard-mh commented 2 years ago

Do you mean we need to exec in code and gateway by code extension and gateway plugin?

gp cli <-> supervisor <-> code / jetbrains

it will be complex, and not support for custom ide

akosyakov commented 2 years ago

Do you mean we need to exec in code and gateway by code extension and gateway plugin?

We don't need to exec anything. VS Code extenison and JB backend plugin already have enough access to internals to do the proper thing.

it will be complex, and not support for custom ide

It will be easier since there is not undocumented assumptions just one interface to implement for custom IDEs.

andreafalzetti commented 2 years ago

We should not run anything in supervisor. It works following:

* VS Code Window subscribes to supervisor to handle notifications

* VS Code Window gets a focus and mark itself as an active client on supervisor

* gp sends open notify request to supervisor

* supervisor detects VS Code Window as an active client

* supervisor propagates open notify request to VS Code Window

* VS Code Window handles the notify request internally and responds when it is done

* supervisor returns to gp

* gp finishes

* VS Code Window loses a focus and mark itself as an **inactive** client on supervisor

This approach makes sense to me, thanks @akosyakov

Regarding the new endpoints open and preview in supervisor - does it make sense to you to have them in control.proto instead of notification.proto?

Update: actually, it makes more sense to have those in notification considering that they will cause a notification to be sent

mustard-mh commented 2 years ago

Proto approach

akosyakov commented 2 years ago

Regarding the new endpoints open and preview in supervisor - does it make sense to you to have them in control.proto instead of notification.proto? Update: actually, it makes more sense to have those in notification considering that they will cause a notification to be sent

Yes, an idea was to extend existing APIs with new kind of notifications.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

loujaybee commented 2 years ago

Not sure if related (or fix in a separate ticket). But a comment from Discord:

Has anyone gotten idea-cli open to work? Git uses $GIT_EDITOR env to open a file when you do rebase -i or commit --amend etc. This is set to: /ide-desktop/bin/idea-cli open --wait on gitpod. I get an IDE notification that a file is supposed to be open, but nothing actually happens. Calling idea-cli open manually from the CLI also does not seem to work.