-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[server] add editor prefix to choose custom IDE #9467
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
Conversation
|
/werft run with-vm 👎 cannot start job - please talk to whoever's in charge of your Werft installation |
|
/werft run with-vm 👍 started the job as gitpod-build-hw-9434-open-via-url.3 |
6192dd1 to
d6926f7
Compare
d6926f7 to
08c92e6
Compare
|
/werft run 👍 started the job as gitpod-build-hw-9434-open-via-url.15 |
| export namespace WithReferrerContext { | ||
| export function is(context: any): context is WithReferrerContext { | ||
| return context && "referrer" in context; | ||
| return context && "referrer" in context && !("ide" in context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&& !WithEditorContext.is(context)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using #referrer and remove checkes above? #editor likes like referrer..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate please? I did not grasp an idea, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#editor works like #referrer we can extend #referrer and remove #editor.
Now #referrer only works for client (referrer) vscode vscode-insiders and jetbrains-gateway, and IDE option can only be one of these below 👇 .
| desktopIDEs: ["intellij", "goland", "pycharm", "phpstorm"] |
We can remove this check and make referrer to be anything like
bowser-extension dashboard-project readme etc and IDEs with any IDE. They are worth tracking too
Or extend ide-configmap to support browser-extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or with #editor if referrer is not define, we do not send track here, and consider #editor as make to choose IDE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR introduces the editor parameter, which have three parameters, ide, version, and referrer, but except ide all of them are optional
So it can be used to solve #7525, just use #ediror:code/http://xxxx
It can also be used to solve #9434 with just #editor:code:stable:browser-extension/http://xxxx
It can also be used to solve any need to temporarily switch IDEs, for example, we may introduce another feature later, where we can hold down the option key in the Open workspace (select url) to temporarily select the current open IDE #9433 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's solve one issue at the time instead of use one technical solution for everything, and don't be to worry about all possible issues which we will need to solve somewhen 🙏
It can also be used to solve #9434 with just #editor:code:stable:browser-extension/http://xxxx
Let's use referrer still there, but introduce a new kind browser-extension which allows any IDE and use code as default, i.e. referrer:browser-extension:code We don't want to allow users to specify random referrers. Please also prioritise browser-extension support as minor. It is not a primary onboarding experience.
So it can be used to solve #7525, just use #ediror:code/http://xxxx
We don't need uncontrolled referrer here at all. It also not nice syntax I would go with something like #editor:code&referrer=.../https://, but then it is better to use referrer prefix with new kinds. I suggest we go with an initial proposal here editor:<ide>(:<version>)?. Adding analytics is not bad then to see whether it is used, but it should not be mixed with ide_referrer event anymore then, something like ide_editor_prefix.
It can also be used to solve any need to temporarily switch IDEs, for example, we may introduce another feature later, where we can hold down the option key in the Open workspace (select url) to temporarily select the current open IDE #9433 (comment)
From the dashboard, we should not rely on prefix parsers, but improve APIs to allow passing concrete IDE to create or start workspace methods. This one actually sounds like most important issue of all mentioned above. But should rather align with @loujaybee and @andreafalzetti what is goal for onboarding right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we go with an initial proposal here editor:(:)?
Go with this. will create a PR to open workspace with custom IDE via API later.
If PR above is created, and PR to add referrer browser-extension is created, why we need this PR? No one will use it.🙈
Feel free to close this PR @akosyakov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If PR above is created, and PR to add referrer browser-extension is created, why we need this PR? No one will use it.🙈
- People will add to README if a project is configured only to work nicely with VS Code for instance.
- I already would like to use it on gitpod-io/gitpod, since I usually have IntelliJ one, but would like to have a workspace only with VS Code 🙏
I think we will need to add some docs about this prefix somewhere. @loujaybee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry I misunderstand. You say you will do 2 PRs instead? Then yes just close this PR and open 2 others.
08c92e6 to
1bcf2a0
Compare
1bcf2a0 to
2104a66
Compare
|
I have a question: |
geropl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the general question "what is meant to be part of the start workpace URL": We should really have a basic test converage before merging a ContextParser.
Two things I would be interested in:
- the "internal" stuff works, and does not throw exceptions (extensive)
- the "external" facet: it does play nicely with other context parsers, e.g. EnvVars (some examples)
@jeanp413 I think this question should back to why we use fragment for In my opinion, we may just want to split input into |
|
Close, will create a new one for it in the future |
Description
Support choose custom IDE with prefix
/#editor:<ide>:<version?>/<repo>codegoland...Related Issue(s)
Fixes #9434
https://github.com/gitpod-io/template-sveltejs
How to test
Open with
Release Notes
Documentation
/werft analytics=segment|TEZnsG4QbLSxLfHfNieLYGF4cDwyFWoe