Skip to content

Conversation

@mustard-mh
Copy link
Contributor

Description

Related Issue(s)

Fixes #

How to test

Documentation

/hold

@mustard-mh mustard-mh requested a review from akosyakov July 17, 2023 10:17
@mustard-mh mustard-mh marked this pull request as draft July 17, 2023 10:20
@mustard-mh
Copy link
Contributor Author

mustard-mh commented Jul 17, 2023

It's hard to say it works with backward compatible (ContextURL.getNormalizedURL does return ok), but at least UI won't break
image

image

cc @akosyakov

@mustard-mh mustard-mh marked this pull request as ready for review July 17, 2023 12:33
}
} catch (e) {
// ignore
return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

it will be unexpected? probably better to log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to add, but it seems needs HostService\LogService\TelemetryService here and change lots of files so that we can send exception. Maybe we will need inject in repo? I'd like to keep it and add TODO as it's right now.

cc @jeanp413 for Maybe we will need inject in repo? this question

image

@mustard-mh mustard-mh merged commit 9b83f25 into master Jul 17, 2023
@jeanp413
Copy link
Member

@mustard-mh Why this got merged xD, workaround from Anton should be enough maybe some 💄 but we need to discuss with Sven or Gero first about fix in server side, personally I don't think raw workspace url should be exposed in public api

jeanp413 added a commit that referenced this pull request Jul 17, 2023
@jeanp413
Copy link
Member

I'll revert for now and we can fix properly after public api is updated

@jeanp413 jeanp413 deleted the hw/context-ur branch July 17, 2023 18:57
@akosyakov
Copy link
Member

workaround from Anton should be enough maybe some
@jeanp413 my work ardoun does not fix anything for UI, onyl for one particular path, UI is failing right now whenever you start a workspace from prebuild and so on. Discussions about Public API will take a while, many people are having time off now. And they may not lead to what you expect.

@jeanp413
Copy link
Member

@jeanp413 my work ardoun does not fix anything for UI, onyl for one particular path, UI is failing right now whenever you start a workspace from prebuild and so on.

yeah I see, I'm working on change that does not touch public api and instead normalizes on client side by removing prefix

@akosyakov
Copy link
Member

akosyakov commented Jul 17, 2023

yeah I see, I'm working on change that does not touch public api and instead normalizes on client side by removing prefix

We already have this code in gitpod-protocol. Coudl we keep it in one place than implement a custom logic? That it is exactly the same logic as on the dashboard. I'm kind of confused what was wrong with Huiwen's PR 🤔 It seems to do the job already and aligned with other clients.

@jeanp413
Copy link
Member

I'm kind of confused what was wrong with Huiwen's PR

it's using the normalizedContextUrl, and unless it's already decided that we'll keep both contextUrl and normalizedContextUrl I prefer not to add it for now beucase then we have to mantain it

We already have this code in gitpod-protocol. Coudl we keep it in one place than implement a custom logic?

I took a look at it but it's buggy as it does not take into account open-prebuild prefix, in any case it can be simplified to just search for http[s]? as all context parsers rely on / as separator [1] [2] [3] [4] [5]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants