Skip to content

Fix Supervisor sending empty port names and descriptions #9797

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 24, 2022
Merged

Fix Supervisor sending empty port names and descriptions #9797

merged 1 commit into from
May 24, 2022

Conversation

@felladrin
Copy link
Contributor

@felladrin felladrin commented May 5, 2022 •
edited by mustard-mh
Loading

Description

Currently, supervisor send empty name and description of ports to editor(code) which make tooltip in code not works (see also #9262 (comment)).

This PR fix it

Related Issue(s)

#9262 (comment)

How to test

Note that port with range will work only when ports are opened

Init with .gitpod.yml

Watch .gitpod.yml

  • Change .gitpod.yml in your workspace, the tooltip in port explorer should reflect to yml file
Image
image

Release Notes

Fix port name and description not showing up on Gitpod VS Code Extension sidebar.

Documentation
@felladrin felladrin force-pushed the vn/fix-empty-port-name-description branch 2 times, most recently from 0dc177d to 34df117 Compare May 5, 2022 18:55
@felladrin
Copy link
Contributor Author

The current code is working, as seen in here. What is wrong is that it's not reading the the .gitpod.yml file as it should.
akosyakov
akosyakov reviewed May 6, 2022
View reviewed changes
@felladrin felladrin closed this May 10, 2022
@felladrin felladrin force-pushed the vn/fix-empty-port-name-description branch from 34df117 to 98726d3 Compare May 10, 2022 13:49
@roboquat roboquat added size/XS and removed size/S labels May 10, 2022
@felladrin felladrin reopened this May 10, 2022
@roboquat roboquat added size/S and removed size/XS labels May 10, 2022
@mustard-mh mustard-mh self-assigned this May 18, 2022
@davidwindell davidwindell mentioned this pull request May 20, 2022
Closed
@felladrin
Copy link
Contributor Author

@mustard-mh, just to let you know I'll add more details to the PR description soon, to make it clear what's the problem and what I have already tried to fix it. Hopefully you can help to find the cause ^^
(It stopped working somewhen in the last 3 months.)
@mustard-mh mustard-mh force-pushed the vn/fix-empty-port-name-description branch from 74fa049 to 98c03ec Compare May 24, 2022 05:33
@roboquat roboquat added size/M and removed size/S labels May 24, 2022
@mustard-mh mustard-mh mentioned this pull request May 24, 2022
Open
3 tasks
@mustard-mh mustard-mh marked this pull request as ready for review May 24, 2022 06:09
@mustard-mh mustard-mh requested review from a team May 24, 2022 06:09
@github-actions github-actions bot added team: IDE team: webapp Issue belongs to the WebApp team labels May 24, 2022
iQQBot
iQQBot approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@iQQBot iQQBot left a comment

Choose a reason for hiding this comment

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

LGTM Thanks @mustard-mh
@mustard-mh mustard-mh force-pushed the vn/fix-empty-port-name-description branch from 98c03ec to 073b8e0 Compare May 24, 2022 06:20
@akosyakov
Copy link
Member

Does it work for port ranges too?
@mustard-mh mustard-mh force-pushed the vn/fix-empty-port-name-description branch from 073b8e0 to 3013055 Compare May 24, 2022 06:32
@mustard-mh
Copy link
Contributor

mustard-mh commented May 24, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Does it work for port ranges too?

I think it will after the new push, waiting for new build to test 👍
@mustard-mh
Copy link
Contributor

Ranges will work only when ports are opened. cc @akosyakov

image
@akosyakov
Copy link
Member

akosyakov commented May 24, 2022 •
edited
Loading

Hm, there is no name in the remote explorer view. It is a bug in vscode web. Could you fix it there as well? 🙏
@mustard-mh
Copy link
Contributor

mustard-mh commented May 24, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Hm, there is no name in a view. It is a bug in vscode web. Could you fit there as well? 🙏

What name in a view means?

1111 should be range: 1111 in the screen capture?
@akosyakov
Copy link
Member

akosyakov commented May 24, 2022 •
edited
Loading

but it is another follow up PR, this one is good 👍
akosyakov
akosyakov reviewed May 24, 2022
View reviewed changes
components/supervisor/pkg/ports/ports.go

var public bool
config, kind, exists := pm.configs.Get(mp.LocalhostPort)
if exists {
Copy link
Member

@akosyakov akosyakov May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder why it is necessary here, configurations should be handled above at step 2, this step about served state
Copy link
Member

Choose a reason for hiding this comment

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

ah, it is for ranges?
Copy link
Member

Choose a reason for hiding this comment

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

Do we miss here anything else like visibility handling or action on expose?

It can be another PR too.
Copy link
Contributor

Choose a reason for hiding this comment

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

visibility (port private and public) is handled here

gitpod/components/supervisor/pkg/ports/ports.go

Line 430 in 3013055

mp.AutoExposure = pm.autoExpose(ctx, mp.LocalhostPort, public).state

Copy link
Member

@akosyakov akosyakov May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to call mp.OnExposed = getOnExposedAction(config, port)? or it is handled already somewhere too?
Copy link
Contributor

@mustard-mh mustard-mh May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to call mp.OnExposed = getOnExposedAction(config, port)? or it is handled already somewhere too?

added

I tested with onOpen: open-browser with ranges before and it works fine, but I don't know where is code. Add to confirm it
Copy link
Member

Choose a reason for hiding this comment

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

thank you, great 👍
Copy link
Member

Choose a reason for hiding this comment

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

maybe a commit in if will be nice, that we are handling configuration for port ranges here
@mustard-mh mustard-mh force-pushed the vn/fix-empty-port-name-description branch from 3013055 to cf43f79 Compare May 24, 2022 07:13
@roboquat roboquat merged commit 37a1d8e into main May 24, 2022
@roboquat roboquat deleted the vn/fix-empty-port-name-description branch May 24, 2022 07:29
@mustard-mh mustard-mh mentioned this pull request May 24, 2022
Merged
1 task
@roboquat 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 May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployed: IDE IDE change is running in production deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/M team: IDE team: webapp Issue belongs to the WebApp team

6 participants

@felladrin @akosyakov @mustard-mh @easyCZ @iQQBot @roboquat