Skip to content

[gp-cli] add command to change ports visibility #13253

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

Merged
merged 1 commit into from
Sep 29, 2022
Merged

[gp-cli] add command to change ports visibility #13253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions components/gitpod-cli/cmd/ports-visibility.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Copyright (c) 2022 Gitpod GmbH. All rights reserved.
// Licensed under the GNU Affero General Public License (AGPL).
// See License-AGPL.txt in the project root for license information.

package cmd

import (
"context"
"fmt"
"log"
"strconv"
"strings"
"time"

gitpod "github.com/gitpod-io/gitpod/gitpod-cli/pkg/gitpod"
serverapi "github.com/gitpod-io/gitpod/gitpod-protocol"
"github.com/spf13/cobra"
)

// portsVisibilityCmd change visibility of port
var portsVisibilityCmd = &cobra.Command{
Use: "visibility <port:{private|public}>",
Short: "Make a port public or private",
Args: cobra.ExactArgs(1),
Run: func(cmd *cobra.Command, args []string) {
portVisibility := args[0]
s := strings.Split(portVisibility, ":")
if len(s) != 2 {
log.Fatal("cannot parse args, should be something like `3000:public` or `3000:private`")
Copy link
Member

Choose a reason for hiding this comment

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

nit(non-blocking): I wonder why we ended up with this format of PORT:VISIBILITY. It reminds me of port bindings in Docker or SSH port-forwarding, but what I expected would be just delimited with a space, like

gp ports visibility 3000 public

what I would find handy and would love to have an alias to this visibility command (maybe even rename it altogether 😝) is to make the syntax as follows:

gp ports make 3000 public

Why?

  • the command reads well and if someone were to dictate it to you over the phone, it'd be hard to "mess it up"
  • it's 6 characters shorter
  • doesn't use any special characters

Copy link
Member

Choose a reason for hiding this comment

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

Tagging @akosyakov as you were the first I could find to propose this format 👀 [1]

Copy link
Contributor

Choose a reason for hiding this comment

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

"make port 3000 public" is very readable however, make is also quite generic and less intuitive at a glance that it's related to setting the port's visibility.

I am happy with the current approach, but happy if we discuss alternatives. My thoughts so far have been:

We could consider set-visibility but I am not fully convinced because:

  1. makes the cmd longer to type (which is not a deal breaker if the name makes sense)
  2. it's not consistent with other cmds (such as gp timeout). For the timeout subcommand we've used the verbs extend and show which at the same time, would make me wonder if the command introduced by this PR, should be gp ports visibility set (although we'd be back at point 1 --> longer cmd).

I then wondered how codespaces named this command and found the same approach so I think @mustard-mh could have used that as inspiration?

gp ports visibility 3000:public is good enough in a way because is short and clear.

The bit that I am not 100% convinced with it either, is that I feel that it's missing the "set" semantic and joining port + visibility feels a bit hacky.

If I had to provide an alternative that I both like and think it's very flexible and clear, it would be: gp ports set-visibility --port 3000 --visibility public - however it's way longer and harder to remember probably.

I am curious to see what other people think.

Copy link
Contributor

@andreafalzetti andreafalzetti Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

To recap, we need to agree on the name of the command and how to provide the values.

For the name, our options are:

1️⃣ gp ports visibility
2️⃣ gp ports visibility set
3️⃣ gp ports set-visibility
4️⃣ gp ports publish and gp ports hide

For providing values:

1️⃣ 3000:public
2️⃣ 3000 public
3️⃣ --port 3000 --visibility public

Any other options or suggestions? Please add or express your preferred way 🙏

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for sharing, Andrea! Agreed make is very generic in commands, so may not be the best option. Just had another idea: gp ports publish 3000 and gp ports private/hide 3000 i.e. - make the publish states verbs.

Copy link
Contributor

@andreafalzetti andreafalzetti Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated my comment above with publish/hide 👍

Naming is always hard but I am sure we will agree as a team on a good name. I think finding the best name for a command is important as much as creating a nice UI on the IDE plugin. Thanks for caring and sparking the discussion :)

Copy link
Member

Choose a reason for hiding this comment

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

Codespaces uses the colon format gh codespace ports visibility codespace-port:private|org|public -c codespace-name so maybe it makes sense
I was thinking of something like gp ports update 3000 --visibility=public if we want to change more attributes for a port in the future like adding a label to it you can set that in vscode

Copy link
Contributor Author

@mustard-mh mustard-mh Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd like to unhold this PR, created an issue for our discussion. #13443 (update its description if you want)

🙏 Thanks folks for your thoughts

}
port, err := strconv.Atoi(s[0])
if err != nil {
log.Fatal("port should be integer")
}
visibility := s[1]
if visibility != serverapi.PortVisibilityPublic && visibility != serverapi.PortVisibilityPrivate {
log.Fatalf("visibility should be `%s` or `%s`", serverapi.PortVisibilityPublic, serverapi.PortVisibilityPrivate)
}
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

wsInfo, err := gitpod.GetWSInfo(ctx)
if err != nil {
log.Fatalf("cannot get workspace info, %s", err.Error())
}
client, err := gitpod.ConnectToServer(ctx, wsInfo, []string{
"function:openPort",
"resource:workspace::" + wsInfo.WorkspaceId + "::get/update",
})
if err != nil {
log.Fatalf("cannot connect to server, %s", err.Error())
}
if _, err := client.OpenPort(ctx, wsInfo.WorkspaceId, &serverapi.WorkspaceInstancePort{
Port: float64(port),
Visibility: visibility,
}); err != nil {
log.Fatalf("failed to change port visibility: %s", err.Error())
}
fmt.Printf("port %v is now %s\n", port, visibility)
},
}

func init() {
portsCmd.AddCommand(portsVisibilityCmd)
}
5 changes: 5 additions & 0 deletions components/gitpod-protocol/go/gitpod-service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1766,6 +1766,11 @@ type WorkspaceInstancePort struct {
Visibility string `json:"visibility,omitempty"`
}

const (
PortVisibilityPublic = "public"
PortVisibilityPrivate = "private"
)

// GithubAppConfig is the GithubAppConfig message type
type GithubAppConfig struct {
Prebuilds *GithubAppPrebuildConfig `json:"prebuilds,omitempty"`
Expand Down