Respond PortsStatus with sorted ports #13788

mustard-mh · 2022-10-12T06:23:11Z

Description

Related PR gitpod-io/openvscode-server#444

Related Issue(s)

How to test

Open workspace in preview env with ports opened. Example repo https://github.com/mustard-mh/test/tree/hw/logs-ports
Check if ports are sorted in PortsView
Check if ports are sorted in gp-cli
Update .gitpod.yml ports order should change order in portsView too (if file change is detect, you can change name after order change to see if it is detect or not)

Rules of order

First respect to .gitpod.yml 's ports define
Others' are sorted by port number ASC
Filter with ports that are not served and defined in gitpod.yml

Release Notes

Display sorted ports with both `gp-cli` and VSCode Browser `PortsView`

Documentation

Werft options:

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-integration-tests=all
Valid options are all, workspace, webapp, ide

werft-gitpod-dev-com · 2022-10-12T06:23:19Z

started the job as gitpod-build-hw-port-responsive.6 because the annotations in the pull request description changed
(with .werft/ from main)

akosyakov · 2022-10-12T10:07:19Z

/hold

@mustard-mh we should respect order configured in .gitpod.yml #7764 (comment) that a user can control it

I think the order should be like:

configured ports with order defined as in .gitpod.yml
not configured but served ports sorted numerically
not configured and not served ports anymore sorted numerically (maybe we should just hide them all together?)

You need to create a test project with all these cases. I don't think just sorting numerically is good idea.

mustard-mh · 2022-10-19T10:02:49Z

/werft run

👍 started the job as gitpod-build-hw-port-responsive.16
(with .werft/ from main)

mustard-mh · 2022-10-19T12:16:49Z

/werft run

👍 started the job as gitpod-build-hw-port-responsive.18
(with .werft/ from main)

mustard-mh · 2022-10-19T13:06:03Z

Is this PR need to document / change on website? @loujaybee

components/gitpod-protocol/go/gitpod-service.go

easyCZ · 2022-10-19T13:10:32Z

components/supervisor/pkg/ports/ports-config.go

@@ -184,7 +186,7 @@ func parseWorkspaceConfigs(ports []*gitpod.PortConfig) (portConfigs map[uint32]*
 }

 func parseInstanceConfigs(ports []*gitpod.PortsItems) (portConfigs map[uint32]*gitpod.PortConfig, rangeConfigs []*RangeConfig) {


Could we instead return an ordered list and avoid specifying the sort order as a property on a map of items?

Could we instead return an ordered list and avoid specifying the sort order as a property on a map of items?

I want to do it too (my first implement), but we have ranged ports (i.e. 3000-5000), if someone defined it with large range, the array will be very large too

Is this a concern around the number of bytes needed to store or some other size? I'd expect that the resulting size in bytes would be nearly identical, we're storing n items in both cases.

The extra property of Index probably costs us more as its repeated on each item as opposed to have the sorting implicit which is what a list would do.

Anyhow, just to wanted to flag this up as it felt odd.

# user can define ranged port like this, and ignore if this num of port is validate or not ports: - port: 100001 - port: 1-100000

With Sort property defined, we don't need anything.

But if we use a new variable like SortedPorts array, it will contain 100001 items. Or, you need to check if this port number is inside this range or not everytime, it makes code harder to read and wastes resources while getting the ports list

We could store it as

ports: [{range: {from: 1, to: 10000}, ...}, {range: {from: 10001, to: 10001}, ...}

Which would give us the same result.

Or, you need to check if this port number is inside this range or not everytime

It's this case

Let's leave it open, and make it a follow-up PR if we are going to change it.

cc @akosyakov @iQQBot

mustard-mh · 2022-10-19T13:16:58Z

/hold

To drop debug commit before merged

iQQBot

Test works, but with a small problem

(non-block) If I only update the port order in .gitpod.yml, not change name or add new port, the port order in vscode port view not change, but if I run gp ports list it works, but if I change a port name, or add new port, the vscode port view is works too

mustard-mh · 2022-10-19T15:53:07Z

(non-block) If I only update the port order in .gitpod.yml, not change name or add new port, the port order in vscode port view not change, but if I run gp ports list it works, but if I change a port name, or add new port, the vscode port view is works too

@iQQBot Thanks, I will file an issue to track it 🙏

easyCZ

Code-wise, changes look good. I think it's awkward API-wise to put the sort index on the resulting Port object and using a sorted list would be cleaner but the port logic is in your domain ownership so feel free to go with this approach.

mustard-mh · 2022-10-20T20:00:48Z

/unhold

akosyakov · 2022-10-20T20:28:38Z

components/gitpod-protocol/go/gitpod-service.go

@@ -1756,6 +1756,7 @@ type PortConfig struct {
 	Visibility  string  `json:"visibility,omitempty"`
 	Description string  `json:"description,omitempty"`
 	Name        string  `json:"name,omitempty"`
+	Sort        uint32  `json:"sort,omitempty"`


Is it something only for internal usage of supervisor? I don’t think we should have such property here then. These are types for gitpod config.

cc @mustard-mh could you remove it to avoid confusion please

akosyakov · 2022-10-20T20:32:08Z

@mustard-mh did we consider ports which are not served and not configured but public? We need to show them always as well. Maybe even with higher priority then served but not configured.

mustard-mh · 2022-10-21T05:31:54Z

@mustard-mh did we consider ports which are not served and not configured but public? We need to show them always as well. Maybe even with higher priority then served but not configured.

Yes, we did, but if we consider public means user changed the visibility of port, then private also can be one of them (private -> public -> private) and if user switch not served public to private again, it will disappear. In this case, users may don't know why private sometimes show and sometimes does not.

So, for me, I think we have only two choices:

Don't filter ports
Filter not served ports

PS. change visibility of ports should not adjust their sort, in case users click so fast (prefer to change them in the same position

cc @akosyakov

mustard-mh · 2022-10-21T06:38:16Z

@akosyakov I created a follow-up PR, to move sort property exists in supervisor only. To address #13788 (comment) and don't filter not served ports

mustard-mh requested a review from a team October 12, 2022 06:23

roboquat added the release-note label Oct 12, 2022

roboquat added the size/XS label Oct 12, 2022

roboquat added the do-not-merge/hold label Oct 12, 2022

mustard-mh marked this pull request as draft October 12, 2022 15:19

roboquat added the do-not-merge/work-in-progress label Oct 12, 2022

mustard-mh force-pushed the hw/port-responsive branch from 36ab232 to 76c6b74 Compare October 14, 2022 06:32

roboquat added size/M and removed size/XS labels Oct 14, 2022

mustard-mh force-pushed the hw/port-responsive branch from 76c6b74 to a79c2ca Compare October 14, 2022 09:29

roboquat added size/L and removed size/M labels Oct 14, 2022

mustard-mh force-pushed the hw/port-responsive branch 4 times, most recently from 8af84aa to ba6af3e Compare October 14, 2022 21:13

mustard-mh mentioned this pull request Oct 17, 2022

Use ports order from supervisor gitpod-io/openvscode-server#444

Merged

mustard-mh added 3 commits October 19, 2022 09:13

[supervisor] respond PortsStatus with order

64c0798

[supervisor] add test cases for ports order

137363e

DEBUG: update code image to use order from supervisor

54f0678

mustard-mh force-pushed the hw/port-responsive branch from 173680d to 54f0678 Compare October 19, 2022 09:14

[gp-cli] remove sort in gp ports list cmd

2569216

mustard-mh marked this pull request as ready for review October 19, 2022 12:59

mustard-mh requested a review from a team October 19, 2022 12:59

roboquat removed the do-not-merge/work-in-progress label Oct 19, 2022

github-actions bot added the team: IDE label Oct 19, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Oct 19, 2022

iQQBot self-assigned this Oct 19, 2022

easyCZ reviewed Oct 19, 2022

View reviewed changes

components/gitpod-protocol/go/gitpod-service.go Show resolved Hide resolved

easyCZ reviewed Oct 19, 2022

View reviewed changes

iQQBot approved these changes Oct 19, 2022

View reviewed changes

akosyakov mentioned this pull request Oct 20, 2022

Remote Explorer should list ports in numerical order #7764

Closed

easyCZ approved these changes Oct 20, 2022

View reviewed changes

roboquat removed the do-not-merge/hold label Oct 20, 2022

roboquat merged commit c2f7cdf into main Oct 20, 2022

roboquat deleted the hw/port-responsive branch October 20, 2022 20:01

mustard-mh mentioned this pull request Oct 20, 2022

Hide unused ports from ports view(s) #10408

Open

akosyakov reviewed Oct 20, 2022

View reviewed changes

mustard-mh mentioned this pull request Oct 21, 2022

[supervisor] localify struct of ports sort #14070

Closed

3 tasks

roboquat added deployed: webapp Meta team change is running in production deployed: IDE IDE change is running in production deployed Change is completely running in production labels Oct 21, 2022

filiptronicek added the component: gp cli label Oct 21, 2022

mustard-mh mentioned this pull request Nov 17, 2022

Improve ports respond in supervisor #14761

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respond PortsStatus with sorted ports #13788

Respond PortsStatus with sorted ports #13788

mustard-mh commented Oct 12, 2022 •

edited

Loading

werft-gitpod-dev-com bot commented Oct 12, 2022

akosyakov commented Oct 12, 2022 •

edited

Loading

mustard-mh commented Oct 19, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mustard-mh commented Oct 19, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mustard-mh commented Oct 19, 2022

easyCZ Oct 19, 2022

mustard-mh Oct 19, 2022 •

edited

Loading

easyCZ Oct 19, 2022

mustard-mh Oct 19, 2022 •

edited

Loading

easyCZ Oct 20, 2022

mustard-mh Oct 20, 2022

mustard-mh Oct 20, 2022

mustard-mh commented Oct 19, 2022 •

edited

Loading

iQQBot left a comment

mustard-mh commented Oct 19, 2022 •

edited by werft-gitpod-dev-com bot

Loading

easyCZ left a comment

mustard-mh commented Oct 20, 2022

akosyakov Oct 20, 2022 •

edited

Loading

akosyakov commented Oct 20, 2022

mustard-mh commented Oct 21, 2022 •

edited

Loading

mustard-mh commented Oct 21, 2022 •

edited

Loading

		@@ -184,7 +186,7 @@ func parseWorkspaceConfigs(ports []gitpod.PortConfig) (portConfigs map[uint32]
		}

		func parseInstanceConfigs(ports []gitpod.PortsItems) (portConfigs map[uint32]gitpod.PortConfig, rangeConfigs []*RangeConfig) {

Respond PortsStatus with sorted ports #13788

Respond PortsStatus with sorted ports #13788

Conversation

mustard-mh commented Oct 12, 2022 • edited Loading

Description

Related Issue(s)

How to test

Rules of order

Release Notes

Documentation

Werft options:

werft-gitpod-dev-com bot commented Oct 12, 2022

akosyakov commented Oct 12, 2022 • edited Loading

mustard-mh commented Oct 19, 2022 • edited by werft-gitpod-dev-com bot Loading

mustard-mh commented Oct 19, 2022 • edited by werft-gitpod-dev-com bot Loading

mustard-mh commented Oct 19, 2022

easyCZ Oct 19, 2022

Choose a reason for hiding this comment

mustard-mh Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

easyCZ Oct 19, 2022

Choose a reason for hiding this comment

mustard-mh Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

easyCZ Oct 20, 2022

Choose a reason for hiding this comment

mustard-mh Oct 20, 2022

Choose a reason for hiding this comment

mustard-mh Oct 20, 2022

Choose a reason for hiding this comment

mustard-mh commented Oct 19, 2022 • edited Loading

iQQBot left a comment

Choose a reason for hiding this comment

mustard-mh commented Oct 19, 2022 • edited by werft-gitpod-dev-com bot Loading

easyCZ left a comment

Choose a reason for hiding this comment

mustard-mh commented Oct 20, 2022

akosyakov Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

akosyakov commented Oct 20, 2022

mustard-mh commented Oct 21, 2022 • edited Loading

mustard-mh commented Oct 21, 2022 • edited Loading

mustard-mh commented Oct 12, 2022 •

edited

Loading

akosyakov commented Oct 12, 2022 •

edited

Loading

mustard-mh commented Oct 19, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mustard-mh commented Oct 19, 2022 •

edited by werft-gitpod-dev-com bot

Loading

mustard-mh Oct 19, 2022 •

edited

Loading

mustard-mh Oct 19, 2022 •

edited

Loading

mustard-mh commented Oct 19, 2022 •

edited

Loading

mustard-mh commented Oct 19, 2022 •

edited by werft-gitpod-dev-com bot

Loading

akosyakov Oct 20, 2022 •

edited

Loading

mustard-mh commented Oct 21, 2022 •

edited

Loading

mustard-mh commented Oct 21, 2022 •

edited

Loading