Skip to content

Improve ports respond in supervisor #14761

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 5 commits into from
Nov 18, 2022
Merged

Improve ports respond in supervisor #14761

merged 5 commits into from
Nov 18, 2022

Conversation

mustard-mh
Copy link
Contributor

@mustard-mh mustard-mh commented Nov 17, 2022 •
edited
Loading

Description

Related Issue(s)

Fixes #14248

How to test

Disable workspace config ports

  • Open workspace with ports already defined in .gitpod.yml
  • Change order after workspace started
  • Check if order align only with current .gitpod.yml

Release Notes

Show not-served ports in gp-cli `ports` command, browser Ports view and desktop ExposedPorts view

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@mustard-mh mustard-mh requested review from a team November 17, 2022 09:23
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-hw-sort-local-2.4 because the annotations in the pull request description changed
(with .werft/ from main)

@github-actions github-actions bot added team: IDE team: webapp Issue belongs to the WebApp team labels Nov 17, 2022
@akosyakov
Copy link
Member

akosyakov commented Nov 17, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Disable workspace config ports

@mustard-mh What does it mean? 🤔

@akosyakov
Copy link
Member

It fixes #14248 right? Since we restore previous contract?

@mustard-mh
Copy link
Contributor Author

mustard-mh commented Nov 17, 2022 •
edited by werft-gitpod-dev-com bot
Loading

It fixes #14248 right? Since we restore previous contract?

Yes, it will, and we are going to introduce new property for hidden state

Disable workspace config ports

@mustard-mh What does it mean? 🤔

We also get ports config from server context, which is constant for a workspace, but it's not necessary because we only detect ports config in current instance (user can modify .gitpod.yml)

If we do not disable it, the order will be weird if user modifies.gitpod.yml

akosyakov
akosyakov reviewed Nov 17, 2022
View reviewed changes
components/gitpod-protocol/go/gitpod-service.go
@@ -1951,7 +1951,6 @@ type PortConfig struct {
Visibility string `json:"visibility,omitempty"`
Description string `json:"description,omitempty"`
Name string `json:"name,omitempty"`
Sort uint32 `json:"sort,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

it should not be here in the first place. cc @geropl @easyCZ for review of webapp owned files

Copy link
Member

Choose a reason for hiding this comment

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

Not mentioned here at all. ✔️

@akosyakov
Copy link
Member

akosyakov commented Nov 17, 2022 •
edited
Loading

@mustard-mh I cannot find test which calls supervisor expose api and ensures that after it there is an event that port is exposed? How we can check some API regression without even calling it?

akosyakov
akosyakov approved these changes Nov 17, 2022
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

looks good, but a regression test ensuring contract for Expose port API is missing

/hold

@mustard-mh
Copy link
Contributor Author

@mustard-mh I cannot test which calls supervisor expose api and ensures that after it there is an event that port is exposed? How we can check some API regression without even calling it?

@akosyakov I add test cases (search Please make sure this test pass for code browser resolveExternalPort) to avoid exposed not served ports not respond

And going to add integration test to make sure it behavior the same with supervisor expose API. Next PR

@mustard-mh
Copy link
Contributor Author

/hold

For me to double check in preview env

geropl
geropl approved these changes Nov 17, 2022
View reviewed changes
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

WebApp changes LGTM.

@jeanp413 jeanp413 mentioned this pull request Nov 17, 2022
Closed
4 tasks
@mustard-mh
Copy link
Contributor Author

/unhold

image

@roboquat roboquat merged commit 975608c into main Nov 18, 2022
@roboquat roboquat deleted the hw/sort-local-2 branch November 18, 2022 10:17
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Nov 18, 2022
@roboquat roboquat added deployed: IDE IDE change is running in production deployed Change is completely running in production labels Nov 18, 2022
@mustard-mh mustard-mh mentioned this pull request Nov 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov approved these changes

@geropl geropl geropl approved these changes

Assignees
No one assigned
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/XL team: IDE team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] asExternalUri never returns
4 participants
@mustard-mh @akosyakov @geropl @roboquat