-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[dev] add gp-gcloud direct replacement for gcloud cli for certain gcloud cli commands #10991
Conversation
09d0455
to
2758b91
Compare
2758b91
to
4fe5f50
Compare
Sorry I missed this issue https://github.com/gitpod-io/ops/issues/2259. Looks like we decided to write a new CLI with the Go library instead of the REST API call comment? |
@jenting yeah. I think it will be better long term then using REST API directly. (As suggested by Alejandro) |
The PR LGTM, thank you 😃 |
gitpod /workspace/workspace-preview/gitpod/dev/gp-gcloud (pavel/gp-gcloud) $ go build
# github.com/gitpod-io/gitpod/gp-gcloud/cmd/compute
cmd/compute/instance-templates-create.go:228:24: undefined: strings.Cut
cmd/compute/instance-templates-create.go:246:24: undefined: strings.Cut
note: module requires Go 1.18
gitpod /workspace/workspace-preview/gitpod (main) $ go version
go version go1.17.5 linux/amd64
gitpod /workspace/workspace-preview/gitpod (main) $ printenv | grep GO
CARGO_HOME=/workspace/.cargo
GOROOT=/home/gitpod/go
GO_VERSION=1.17.5
GOPATH=/workspace/go
PEBKAC. |
Went through all files, except |
Hey @sagor999 👋 , I was comparing the awesome instance template with a real one, and observed some differences. Other feedback incoming via the code! ✉️ edit: @sagor999 wdyt of the differences? I'm thinking we should have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sagor999 no requested changes, just some questions to help me better understand how to test, and how some values were set.
Boot: false, | ||
InitializeParams: &compute.AttachedDiskInitializeParams{ | ||
DiskType: "local-ssd", | ||
Labels: labels, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the label set we cannot set w/o the CLI, right?
Can we test this w/o building an ephemeral cluster? For example, after building this template, can we curl it to determine what these properties should look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can check those labels by inspecting created instance template in gcp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see them in my instance template...can you share a screenie of what you mean given a similar example created with this CLI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about equivalent rest response
, thanks @sagor999 ! 🤝
I was expecting to see labels on both the boot and local SSD disks, but, only see them on boot. 🤔 Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man.
// Labels: Labels to apply to this disk. These can be later modified by
// the disks.setLabels method. This field is only applicable for
// persistent disks.
Labels map[string]string `json:"labels,omitempty"`
Sigh. So I guess labels are ONLY supported for persistent disks. It is a benefit, since once we switch to PVC we will use much less local ssd drives.
But let me look a bit further into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, local ssd drives do not support labels, since they are so ephemeral. Only persistent disks support labels. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated code to reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger that! I just wrapped an IDE incident, and it's time to get ice cream with the Mrs., so I"ll pick this up on Monday. 👋 🍨 🥄
Thank you for the update, bud, have a nice weekend! 🎉
I will double check access scopes, those should have been correct with this |
@kylos101 fixed API scopes. Thank you for catching it! |
👍 scopes seem off still, I might be doing something wrong? They appear to be granting full access. I see this in gen54: And this in awesome template: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @sagor999 ! Appreciate your patience on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet
Description
For reference: https://github.com/gitpod-io/ops/issues/2259
We need to be able to label disks when creating instance templates used by workspace clusters.
gcloud cli
does not support that. REST API or go library does.Using REST API directly would be too bulky and not very maintainable.
Hence
gp-gcloud
cli was created. It acts as a direct drop in replacement for gcloud cli.Currently only supports
compute instance-templates create
command.Otherwise we can still use gcloud for other commands.
Related Issue(s)
Fixes #
How to test
Open this PR in gitpod.
Run this command that will create instance template in
workspace-preview
project (if you have access to it):Release Notes
Documentation
Werft options: