Skip to content

[public-api] define endpoints for ports and git token #15110

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
Dec 6, 2022
Merged

[public-api] define endpoints for ports and git token #15110

merged 1 commit into from
Dec 6, 2022

Conversation

mustard-mh
Copy link
Contributor

@mustard-mh mustard-mh commented Dec 1, 2022 •
edited by jeanp413
Loading

Description

Define endpoints for ports and git token

Release Notes

NONE

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 mentioned this pull request Dec 1, 2022
Merged
17 tasks
@jeanp413 jeanp413 changed the title [public-api] define endpoints for workspace service and git token [public-api] define endpoints for ports and git token Dec 1, 2022
@jeanp413 jeanp413 marked this pull request as ready for review December 1, 2022 19:28
@jeanp413 jeanp413 requested review from a team December 1, 2022 19:28
@github-actions github-actions bot added team: SID team: IDE team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels Dec 1, 2022
akosyakov
akosyakov reviewed Dec 2, 2022
View reviewed changes
components/public-api/gitpod/experimental/v1/workspaces.proto Outdated
@@ -249,12 +251,37 @@ message WorkspaceInstanceStatus {
// Admission describes who can access a workspace instance and its ports.
AdmissionLevel admission = 6;

// ports is the list of served ports in the workspace.
Copy link
Member

Choose a reason for hiding this comment

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

Exposed, not served. It is only about access regardless of whether anyone is listening

@akosyakov
Copy link
Member

@easyCZ could you have a look please?

easyCZ
easyCZ reviewed Dec 2, 2022
View reviewed changes
components/public-api/gitpod/experimental/v1/workspaces.proto
@@ -249,12 +251,37 @@ message WorkspaceInstanceStatus {
// Admission describes who can access a workspace instance and its ports.
AdmissionLevel admission = 6;

// ports is the list of served ports in the workspace.
repeated Port ports = 7;
Copy link
Member

Choose a reason for hiding this comment

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

There's a bit of an assymetry here.

ControlPort(ControlPortRequest) is identified by Workspace ID, but the resulting ports are shown on the Workspace Instance. What limits us from putting the Ports on the WorkspaceStatus rather than the WorkspaceInstanceStatus?

components/public-api/gitpod/experimental/v1/workspaces.proto Outdated
// repo details the Git working copy status of the workspace.
// Note: this is a best-effort field and more often than not will not be present. Its absence does not
// indicate the absence of a working copy.
// contentservice.GitStatus repo = 7;
}

// PortVisibility defines who may access a workspace port which is guarded by an authentication in the proxy
enum PortVisibility {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enum PortVisibility {
enum PortPolicy {

As it feels more appropriate

Copy link
Member

Choose a reason for hiding this comment

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

Another alternative can be PortPrivacy. I think it is what VS Code apis are using, i.e. public, private, team, org and so on

components/public-api/gitpod/experimental/v1/workspaces.proto Outdated
@@ -27,6 +27,8 @@ service WorkspacesService {
// NOT_FOUND: the workspace_id is unkown
// FAILED_PRECONDITION: if there's no running instance
rpc StopWorkspace(StopWorkspaceRequest) returns (stream StopWorkspaceResponse) {}

rpc ControlPort(ControlPortRequest) returns (ControlPortResponse) {}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can call it CreatePortPolicy to remain closer to CRUD

Copy link
Member

Choose a reason for hiding this comment

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

We actually considered to call it UpdatePort.

components/public-api/gitpod/experimental/v1/workspaces.proto Outdated
Comment on lines 274 to 283
message Port {
// port number
uint64 port = 1;

// visiblity of this port
PortVisibility visibility = 2;

// url is the outward-facing URL where the port can be accessed on.
string url = 3;
}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it makes sense to have the following objects to structure the request & response:

message PortSpec {
	 // port number
    uint64 port = 1;

    // visiblity of this port
    PortVisibility visibility = 2;
}

message PortStatus {
	// the URL which can be used to access the port
	string url = 1;
}

They would then be used in the following:

message Workspace {
	...
	repeated Port ports = 10;
}

message WorkspaceStatus {
	repeated PortStatus ports_status = 10;
}

Copy link
Contributor Author

@mustard-mh mustard-mh Dec 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm confused with workspace and workspace instance in Public API, we are going to remove the concept of instance for public API right? @easyCZ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will keep it first, and update it in the future if needed

@easyCZ
Copy link
Member

easyCZ commented Dec 2, 2022

Note: these are suggestions and not necessarily something you are required to do. I'm more than happy to discuss and adjust. I don't always have perfect context on your IDE APIs so some suggestions may not work given the context.

akosyakov
akosyakov approved these changes Dec 5, 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.

/hold

I approved that you don't need to wait for another round to merge. But please have look at Milan's comments. As Milan said we don't need to get it yet 100% right, but would be good to move in the right direction.

@mustard-mh
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit 42d6129 into main Dec 6, 2022
@roboquat roboquat deleted the hw/pub-supervisor branch December 6, 2022 15:37
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed: IDE IDE change is running in production labels Dec 7, 2022
@jeanp413 jeanp413 mentioned this pull request Dec 7, 2022
Merged
4 tasks
@roboquat roboquat added the deployed: workspace Workspace team change is running in production label Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@easyCZ easyCZ easyCZ left review comments

@akosyakov akosyakov akosyakov approved these changes

@csweichel csweichel Awaiting requested review from csweichel

Assignees
No one assigned
Labels
deployed: IDE IDE change is running in production deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production release-note-none size/L team: IDE team: SID team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mustard-mh @akosyakov @easyCZ @roboquat