Skip to content

[supervisor] localify struct of ports sort #14070

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

Closed
wants to merge 3 commits into from
Closed

Conversation

mustard-mh
Copy link
Contributor

@mustard-mh mustard-mh commented Oct 21, 2022

Description

Related Issue(s)

Fixes #

How to test

Release Notes

NONE

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
Copy link

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

@mustard-mh
Copy link
Contributor Author

mustard-mh commented Oct 21, 2022

/werft run

👍 started the job as gitpod-build-hw-sort-local.2
(with .werft/ from main)

continue
}
res = append(res, portStatus)
res = append(res, pm.getPortStatus(port))
Copy link
Member

Choose a reason for hiding this comment

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

Filtering maybe is alright just we need to keep public ports always

Copy link
Contributor Author

@mustard-mh mustard-mh Oct 21, 2022

Choose a reason for hiding this comment

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

It means, port will be gone if user switches a public not served port to private. 🤔

see also #13788 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let’s keep it.

Another approach it is to filter ports which can be only local, ie nodejs debug ports. But I’m not sure how it can achieved on supervisor level. VS Code extensions certainly know it.

@mustard-mh
Copy link
Contributor Author

Tested with not served, functions of ports and gp ports commands

image

@mustard-mh mustard-mh marked this pull request as ready for review October 21, 2022 07:33
@mustard-mh mustard-mh requested review from a team October 21, 2022 07:33
@github-actions github-actions bot added team: IDE team: webapp Issue belongs to the WebApp team labels Oct 21, 2022
@akosyakov
Copy link
Member

I removed some ports from .gitpod.yml expecting remaining to be put in front but nothing happened
Screenshot 2022-10-21 at 09 46 08

@akosyakov
Copy link
Member

@mustard-mh Did we add unit tests for sorting? Why don't they catch cases when configured ports are changing?

@mustard-mh
Copy link
Contributor Author

mustard-mh commented Oct 21, 2022

@akosyakov It should work now after the last commit.

cc @iQQBot this fixes the issue you faced

@mustard-mh
Copy link
Contributor Author

/hold

To squash commits

@roboquat roboquat added size/XL and removed size/L labels Oct 21, 2022
@mustard-mh
Copy link
Contributor Author

mustard-mh commented Oct 21, 2022

/werft run

👍 started the job as gitpod-build-hw-sort-local.6
(with .werft/ from main)

@mustard-mh
Copy link
Contributor Author

mustard-mh commented Oct 21, 2022

To reduce conflict, I created a temp PR #14079 (see its last commit) to remove worksaceConfig in ports.

Conflict i.e. ports (a,b,c) defined in workspace start but removed (a) in instance, a will still be on top of the ports list, this is not correct

We can only respond with ports that exist in the current instance's .gitpod.yml

@akosyakov Is it good for you? then I will cherry-pick it in this PR, squash commits, and mark this PR as ready

@akosyakov
Copy link
Member

akosyakov commented Oct 21, 2022

Don't understand. This PR fixes unnecessary changes to Gitpod protocol? And we fix order with another PR? It is fine with me.

@mustard-mh
Copy link
Contributor Author

mustard-mh commented Oct 21, 2022

@akosyakov We have watched (.gitpod.yml) config changes in supervisor. config will be parsed into workspaceConfigs, instancePortConfigs and instanceRangeConfigs.

  • Our changes in .gitpod.yml will only affect instancePortConfigs and instanceRangeConfigs.
  • workspaceConfigs only be parsed from workspace info that comes from server.

That means, if we have a branch, with port 3000 defined in .gitpod.yml, open a workspace and change .gitpod.yml ports to have 8080 only, port 3000 will still be considered as defined in .gitpod.yml and exists on the top on ports list.

So, we can remove everything related to workspaceConfigs because it's already not the current state of the config.

I don't quite sure if you agree with it (remove workspaceConfigs), so I make it another PR, which only adds a commit to remove it compare with the current PR

@stale
Copy link

stale bot commented Nov 1, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Nov 1, 2022
@stale stale bot closed this Nov 9, 2022
@mustard-mh mustard-mh deleted the hw/sort-local branch December 15, 2022 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants