Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 7 additions & 4 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ jobs:
- name: Free disk space
run: |
docker system prune -a -f
sudo rm -rf /usr/local/lib/android
Comment thread
glasnt marked this conversation as resolved.
df /var/
df -h
- uses: actions/checkout@master
- uses: actions/setup-go@v1
- uses: actions/setup-go@v5
with:
go-version: 1.14
go-version: 1.21
- run: git config --global init.defaultBranch main # TODO remove later as git updates to a newer version
- run: go mod download
- name: Set up git
Expand All @@ -26,13 +28,14 @@ jobs:
working-directory: ./cmd/cloudshell_open
- name: Build docker image
run: docker build -t runbutton .

redirector:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@master
- uses: actions/setup-go@v1
- uses: actions/setup-go@v5
with:
go-version: 1.14
go-version: 1.21
- run: go mod download
working-directory: ./cmd/redirector
- name: Validate formatting with go fmt
Expand Down
4 changes: 2 additions & 2 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM golang:1.14-alpine AS build
FROM golang:1.21-alpine AS build
RUN apk add --no-cache git
WORKDIR /src
COPY go.mod go.sum ./
Expand All @@ -8,4 +8,4 @@ RUN CGO_ENABLED=0 GOOS=linux go build -a -ldflags '-extldflags "-static"' \
-o /bin/a.out ./cmd/cloudshell_open

FROM gcr.io/cloudshell-images/cloudshell:latest
COPY --from=build /bin/a.out /bin/cloudshell_open
COPY --from=build /bin/a.out /bin/cloudshell_open
Comment thread
glasnt marked this conversation as resolved.
74 changes: 74 additions & 0 deletions cmd/cloudshell_open/artifactregistry.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package main

import (
"context"
"errors"
"fmt"

artifactregistry "cloud.google.com/go/artifactregistry/apiv1"
artifactregistrypb "cloud.google.com/go/artifactregistry/apiv1/artifactregistrypb"
codes "google.golang.org/grpc/codes"
status "google.golang.org/grpc/status"
)

// Create a "Cloud Run Source Deploy" repository in Artifact Registry (if it doesn't already exist)
func createArtifactRegistry(project string, region string, repoName string) error {

repoPrefix := fmt.Sprintf("projects/%s/locations/%s", project, region)
repoFull := fmt.Sprintf("%s/repositories/%s", repoPrefix, repoName)

ctx := context.Background()

client, err := artifactregistry.NewClient(ctx)
if err != nil {
return fmt.Errorf("failed to create artifact registry client: %w", err)
}

// Check for existing repo
req := &artifactregistrypb.GetRepositoryRequest{
Name: repoFull,
}
existingRepo, err := client.GetRepository(ctx, req)

if err != nil {
// The repo might not already exist, so allow that specific grpc error
notFoundError := status.Error(codes.NotFound, "Requested entity was not found.")
if !(errors.Is(err, notFoundError)) {
return fmt.Errorf("failed to retrieve existing artifact registry client: %w", err)
}
}

// If the existing repo doesn't exist, create it
if existingRepo == nil {
req := &artifactregistrypb.CreateRepositoryRequest{
Parent: repoPrefix,
RepositoryId: repoName,
Repository: &artifactregistrypb.Repository{
Name: repoFull,
Format: artifactregistrypb.Repository_DOCKER,
},
}

_, err := client.CreateRepository(context.TODO(), req)
if err != nil {
return fmt.Errorf("failed to create artifact registry: %w", err)
}
}

return nil

}
Comment thread
glasnt marked this conversation as resolved.
12 changes: 6 additions & 6 deletions cmd/cloudshell_open/cloudrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,12 @@ func envVars(project, name, region string) (map[string]struct{}, error) {
// prevent deployment failures due to Cloud Run service naming constraints such
// as:
//
// * names with a leading non-letter (e.g. digit or '-') are prefixed
// * names over 63 characters are truncated
// * names ending with a '-' have the suffix trimmed
func tryFixServiceName(name string) string {
// - names with a leading non-letter (e.g. digit or '-') are prefixed
// - names over 63 characters are truncated
// - names ending with a '-' have the suffix trimmed
func tryFixServiceName(name string) (string, error) {
if name == "" {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Being very nitpicky here, but this seems like this would be an erroneous situation, and a function name that starts with try sort of implies to me that it should return errors for unexpected things (maybe just baggage from working with too many languages with try/catch), but otherwise, why even guard here? Either we never expect this to happen or else it's probably better to fail here than when creating the image fails with an empty service name.

Maybe better if we have this?

func tryFixServiceName(name string) (string, error) {
        if name == "" {
                return "", fmt.Errorf("service name can't be empty")
        }

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There's a lot of older code here that I don't think would pass Golang standards today. The above diff is specifically the output of a linting command, which happens to be near one of the many other issues. I won't be addressing all of those in this PR

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry, conversation isn't resolved yet. I think you thought I was commenting on the lint recommendations. I was commenting on and suggesting that tryServiceName actually return an error rather than accept and return an (erroneous) empty string. I realize you didn't touch this code, my suggestion is just based on us leaving code in a better state than we found it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @glasnt, the rest of the PR LGTM and I don't want to be too nitpicky. I asked @telpirion to take a look at this and resolve the conversation accordingly.

return name
return "", fmt.Errorf("service name can't be empty")
}

name = strings.ToLower(name)
Expand All @@ -145,5 +145,5 @@ func tryFixServiceName(name string) string {
name = name[:len(name)-1]
}

return name
return name, nil
}
2 changes: 1 addition & 1 deletion cmd/cloudshell_open/cloudrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func Test_tryFixServiceName(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := tryFixServiceName(tt.in); got != tt.want {
if got, _ := tryFixServiceName(tt.in); got != tt.want {
t.Errorf("tryFixServiceName(%s) = %v, want %v", tt.in, got, tt.want)
}
})
Expand Down
20 changes: 17 additions & 3 deletions cmd/cloudshell_open/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const (
billingCreateURL = "https://console.cloud.google.com/billing/create"
trygcpURL = "https://console.cloud.google.com/trygcp"
instrumentlessEvent = "crbutton"
artifactRegistry = "cloud-run-source-deploy"
)

var (
Expand Down Expand Up @@ -276,7 +277,7 @@ func run(opts runOpts) error {
fmt.Sprintf("Enabling Cloud Run API on project %s...", highlight(project)),
fmt.Sprintf("Enabled Cloud Run API on project %s.", highlight(project)),
fmt.Sprintf("Failed to enable required APIs on project %s.", highlight(project)))
err = enableAPIs(project, []string{"run.googleapis.com", "containerregistry.googleapis.com"})
err = enableAPIs(project, []string{"run.googleapis.com", "artifactregistry.googleapis.com"})
end(err == nil)
if err != nil {
return err
Expand All @@ -291,14 +292,27 @@ func run(opts runOpts) error {
}
}

end = logProgress(
fmt.Sprintf("Setting up %s in region %s (if it doesn't already exist)", highlight(artifactRegistry), highlight(region)),
fmt.Sprintf("Set up %s in region %s (if it doesn't already exist)", highlight(artifactRegistry), highlight(region)),
"Failed to setup artifact registry.")
err = createArtifactRegistry(project, region, artifactRegistry)
Comment thread
glasnt marked this conversation as resolved.
end(err == nil)
if err != nil {
return err
}

repoName := filepath.Base(appDir)
serviceName := repoName
if appFile.Name != "" {
serviceName = appFile.Name
}
serviceName = tryFixServiceName(serviceName)
serviceName, err = tryFixServiceName(serviceName)
if err != nil {
return err
}

image := fmt.Sprintf("gcr.io/%s/%s", project, serviceName)
image := fmt.Sprintf("%s-docker.pkg.dev/%s/%s/%s", region, project, artifactRegistry, serviceName)

existingEnvVars := make(map[string]struct{})
// todo(jamesward) actually determine if the service exists instead of assuming it doesn't if we get an error
Expand Down
56 changes: 47 additions & 9 deletions go.mod
Original file line number Diff line number Diff line change
@@ -1,18 +1,56 @@
module github.com/GoogleCloudPlatform/cloud-run-button

go 1.14
go 1.21

toolchain go1.21.0

require (
cloud.google.com/go/compute v0.1.0
cloud.google.com/go/artifactregistry v1.15.0
cloud.google.com/go/compute/metadata v0.5.0
github.com/AlecAivazis/survey/v2 v2.0.7
github.com/Netflix/go-expect v0.0.0-20200312175327-da48e75238e2 // indirect
github.com/briandowns/spinner v1.9.0
github.com/fatih/color v1.9.0
github.com/fatih/color v1.15.0
google.golang.org/api v0.193.0
)

require (
cloud.google.com/go v0.115.1 // indirect
cloud.google.com/go/auth v0.9.0 // indirect
cloud.google.com/go/auth/oauth2adapt v0.2.4 // indirect
cloud.google.com/go/iam v1.1.13 // indirect
cloud.google.com/go/longrunning v0.5.12 // indirect
github.com/Netflix/go-expect v0.0.0-20200312175327-da48e75238e2 // indirect
github.com/creack/pty v1.1.9 // indirect
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/go-logr/logr v1.4.2 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/google/s2a-go v0.1.8 // indirect
github.com/google/uuid v1.6.0 // indirect
github.com/googleapis/enterprise-certificate-proxy v0.3.2 // indirect
github.com/googleapis/gax-go/v2 v2.13.0 // indirect
github.com/hinshun/vt10x v0.0.0-20180809195222-d55458df857c // indirect
github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect
github.com/kr/pty v1.1.8 // indirect
github.com/kr/text v0.2.0 // indirect
github.com/mattn/go-colorable v0.1.6 // indirect
github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
google.golang.org/api v0.65.0
gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect
github.com/mattn/go-colorable v0.1.13 // indirect
github.com/mattn/go-isatty v0.0.19 // indirect
github.com/mgutz/ansi v0.0.0-20170206155736-9520e82c474b // indirect
go.opencensus.io v0.24.0 // indirect
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.52.0 // indirect
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.53.0 // indirect
go.opentelemetry.io/otel v1.28.0 // indirect
go.opentelemetry.io/otel/metric v1.28.0 // indirect
go.opentelemetry.io/otel/trace v1.28.0 // indirect
golang.org/x/crypto v0.26.0 // indirect
golang.org/x/net v0.28.0 // indirect
golang.org/x/oauth2 v0.22.0 // indirect
golang.org/x/sync v0.8.0 // indirect
golang.org/x/sys v0.24.0 // indirect
golang.org/x/text v0.17.0 // indirect
golang.org/x/time v0.6.0 // indirect
google.golang.org/genproto v0.0.0-20240814211410-ddb44dafa142 // indirect
google.golang.org/genproto/googleapis/api v0.0.0-20240814211410-ddb44dafa142 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240814211410-ddb44dafa142 // indirect
google.golang.org/grpc v1.65.0 // indirect
google.golang.org/protobuf v1.34.2 // indirect
)
Loading