Skip to content
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

[supervisor] Provide port Name and Description along with the port status #7012

Merged
merged 1 commit into from
Dec 20, 2021
Merged

[supervisor] Provide port Name and Description along with the port status #7012

merged 1 commit into from
Dec 20, 2021

Conversation

felladrin
Copy link
Contributor

@felladrin felladrin commented Dec 2, 2021

Description

Changes on gitpod-protocol and the supervisor-api to add name and description to PortsStatus message, which carries the values obtained from Gitpod Workspace PortConfig.

Related Issue(s)

Fix #3059

How to test

Run port_test.go, which has one test for the name attribute and another one for the description.

Release Notes

Allow setting a name and a description for each port on .gitpod.yml

@akosyakov
Copy link
Member

akosyakov commented Dec 2, 2021

/werft run

👍 started the job as gitpod-build-port-description-3059-fork.0

@akosyakov
Copy link
Member

@felladrin I tried to run CI but tests are failing: https://werft.gitpod-dev.com/job/gitpod-build-port-description-3059-fork.0 Try to run them locally.

@akosyakov
Copy link
Member

@felladrin It would be nice if you change the description to use the PR template. It is important for automation, i.e. reference an associated issue and provide proper CHANGELOG.

@felladrin
Copy link
Contributor Author

felladrin commented Dec 2, 2021

@felladrin I tried to run CI but tests are failing: https://werft.gitpod-dev.com/job/gitpod-build-port-description-3059-fork.0 Try to run them locally.

Alright! Now I understand how the tests work; gonna try to fix it now.

@felladrin It would be nice if you change the description to use the PR template. It is important for automation, i.e. reference an associated issue and provide proper CHANGELOG.

Hmm, I created the PR via Gitpod workspace, but I don't remember it suggesting a template.
Just in case, I updated the description based on what I saw on other PRs.

@roboquat roboquat added size/XXL and removed size/XL labels Dec 2, 2021
@akosyakov
Copy link
Member

akosyakov commented Dec 3, 2021

/werft run

👍 started the job as gitpod-build-port-description-3059-fork.1

@codecov
Copy link

codecov bot commented Dec 3, 2021

Codecov Report

Merging #7012 (85517fa) into main (4d79bc8) will decrease coverage by 13.28%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #7012       +/-   ##
==========================================
- Coverage   19.04%   5.76%   -13.29%     
==========================================
  Files           2      13       +11     
  Lines         168    1162      +994     
==========================================
+ Hits           32      67       +35     
- Misses        134    1094      +960     
+ Partials        2       1        -1     
Flag Coverage Δ
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
installer-raw-app 5.76% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go
installer/pkg/components/ws-manager/rolebinding.go 0.00% <0.00%> (ø)
installer/pkg/components/ws-manager/tlssecret.go 0.00% <0.00%> (ø)
installer/pkg/common/common.go 4.64% <0.00%> (ø)
installer/pkg/common/objects.go 0.00% <0.00%> (ø)
installer/pkg/components/ws-manager/configmap.go 29.71% <0.00%> (ø)
installer/pkg/common/ca.go 0.00% <0.00%> (ø)
installer/pkg/common/display.go 0.00% <0.00%> (ø)
installer/pkg/common/render.go 0.00% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d79bc8...85517fa. Read the comment docs.

@felladrin felladrin changed the title [supervisor] Send the port description along with the port status [supervisor] Provide port Name and Description along with the port status Dec 12, 2021
@akosyakov
Copy link
Member

akosyakov commented Dec 13, 2021

/werft run

👍 started the job as gitpod-build-port-description-3059-fork.2

@akosyakov
Copy link
Member

Ok, so now a new version port-description-3059-fork of @gitpod/supervisor-api-grpc is published. You should be able to updated it here:
https://github.com/gitpod-io/openvscode-server/blob/4fe2a9f439a590b804691e3b73cf78db03b7b7a6/extensions/gitpod-shared/package.json#L21 to make used in Gitpod VS Code

It is important that VS Code still can run in the cluster where supervisor does not expose such API, so changes should be done in backward compatible manner. The best way to test it a PR in Gitpod VS Code

  • to start it against gitpod.io to verify backward compatibility
  • to start it against port-description-3059-fork.staging.gitpod-dev.com to verify that new APIs are there

If you are blocked in port-description-3059-fork.staging.gitpod-dev.com let me know if will grant you access to this env.

@gtsiolis gtsiolis added the do-not-merge/cla-pending CLA has not been signed label Dec 14, 2021
@gtsiolis
Copy link
Contributor

gtsiolis commented Dec 14, 2021

Thanks for contributing, @felladrin! 🌟

fyi: You'll also need to sign a Contributor License Agreement (CLA) once before merging your first contribution. Looping in @meysholdt to reach out about the CLA. 🏀

@meysholdt
Copy link
Member

Hi Victor,

I've sent you a CLA via DocuSign, can you please sign it so that we can merge this PR?
thank you!

@felladrin
Copy link
Contributor Author

felladrin commented Dec 15, 2021

Hi, guys. I've signed the CLA.

@akosyakov, following your instructions, I was able to test it on gitpod.io, but I couldn't access port-description-3059-fork.staging.gitpod-dev.com (I'm always getting This site can’t be reached [ERR_CONNECTION_CLOSED]).

In any case, I've opened a PR on Gitpod VS Code: gitpod-io/openvscode-server#258 - so we can continue the discussion there.

@akosyakov
Copy link
Member

akosyakov commented Dec 15, 2021

/werft run

👍 started the job as gitpod-build-port-description-3059-fork.3

@felladrin prev env was down

Could you please squash commits. We try to keep history linear and logical. It is fine to have several commits in PR, but in final state commits should be cleaned up. We usually use several commits only when several logical parts are changed or one PR addresses multiple issues. In this case one commit is enough.

@felladrin
Copy link
Contributor Author

Ok, squashed.

By the way, now I'm able to login the prev environment. But received the "blocked" message when trying to open Gitpod VS Code repo there.

@akosyakov
Copy link
Member

akosyakov commented Dec 16, 2021

/werft run

👍 started the job as gitpod-build-port-description-3059-fork.4

@akosyakov
Copy link
Member

/lgtm
/approve

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: b0b5e13c5ab1eeb6b823e3574319f9a8d5eeb17e

@akosyakov
Copy link
Member

akosyakov commented Dec 16, 2021

@gitpod-io/engineering-meta Could you review it please? 🙏

@akosyakov
Copy link
Member

akosyakov commented Dec 16, 2021

/werft run

👍 started the job as gitpod-build-port-description-3059-fork.5

@geropl
Copy link
Member

geropl commented Dec 16, 2021

@felladrin If we add name/description it should be added to the type as well. Unfortunately we do not enforce equivalence between types and schema automatically, yet... 😕

@roboquat roboquat removed the lgtm label Dec 16, 2021
@akosyakov akosyakov requested a review from geropl December 16, 2021 15:57
@geropl
Copy link
Member

geropl commented Dec 17, 2021

/lgtm

Thx @felladrin ! 🙏

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 4351f0003c9f2e8f7f213b69c53d748bfed738ee

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akosyakov, geropl

Associated issue: #3059

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@akosyakov
Copy link
Member

akosyakov commented Dec 20, 2021

/werft run

👍 started the job as gitpod-build-port-description-3059-fork.6

@roboquat roboquat merged commit f854940 into gitpod-io:main Dec 20, 2021
@felladrin felladrin deleted the port-description-3059 branch December 20, 2021 19:36
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Dec 21, 2021
@meysholdt meysholdt added cla: accepted ✔️ and removed do-not-merge/cla-pending CLA has not been signed labels Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ports need descriptions
6 participants