[gp-cli] add command to change ports visibility #13253
Conversation
|
started the job as gitpod-build-hw-cmd-port-visibility.1 because the annotations in the pull request description changed |
|
/werft run 👍 started the job as gitpod-build-hw-cmd-port-visibility.2 |
|
/werft run 👍 started the job as gitpod-build-hw-cmd-port-visibility.3 |
github-actions
bot
added
team: IDE
team: webapp
|
/werft run 👍 started the job as gitpod-build-hw-cmd-port-visibility.4 |
mustard-mh
force-pushed
the
hw/cmd-port-visibility
branch
from
b043304 to
307748f
Compare
|
Updated |
There was a problem hiding this comment.
Thanks a lot for working on this, @mustard-mh 🙏. It's a really great feature I cannot wait to use!
| 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`") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Tagging @akosyakov as you were the first I could find to propose this format 👀 [1]
There was a problem hiding this comment.
"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:
- makes the cmd longer to type (which is not a deal breaker if the name makes sense)
- it's not consistent with other cmds (such as
gp timeout). For thetimeoutsubcommand we've used the verbsextendandshowwhich at the same time, would make me wonder if the command introduced by this PR, should begp 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.
There was a problem hiding this comment.
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 🙏
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
mustard-mh
force-pushed
the
hw/cmd-port-visibility
branch
from
307748f to
f0ae586
Compare
|
Updated help message (see diff) /unhold |
|
@mustard-mh Why don't you use supervisor API? I think we should to manage load in one place. Could you open a follow up PR please. 🙏 |
|
Actually I see we do the same in VS Code. |
roboquat
added
deployed: IDE
Description
Add new gp-cli command to allow users to change ports visibility
Related Issue(s)
Fixes #10835
How to test
Open workspace in preview env, check with portsView to see if port is locked(private) or unlocked(public)
Public
curl lama.sh | shto open port 8080gp ports visibility 8080:publicto make it publicPrivate
gp ports visibility 8080:privateto make it privateRelease Notes
Documentation
Werft options:
If enabled this will build
install/previewValid options are
all,workspace,webapp,ide