feat(storage): add gRPC quickstart sample #4438

frankyn · 2024-10-15T17:05:34Z

Description

Fixes b/373643411

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

Tests pass: go test -v ./.. (see Testing)
Code formatted: gofmt (see Formatting)
Vetting pass: go vet (see Formatting)
Please merge this PR for me once it is approved

snippet-bot · 2024-10-15T18:09:16Z

Here is the summary of changes.

You are about to add 1 region tag.

storage/grpc_quickstart/main.go:15, tag storage_grpc_quickstart

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

tritone

Few minor comments, otherwise lgtm

tritone · 2024-10-15T19:01:10Z

storage/storage_grpc_quickstart/main_test.go

+
+	stdOut, stdErr, err := m.Run(nil, time.Minute, "--project", tc.ProjectID, "--bucket", bucketName)
+
+	if err != nil {


Should we also specifically check for the error about directPath not being enabled or metrics not being enabled? Do we want to try to ensure that these things work properly for the test?

Warning wasn't implemented into the new auth code path introduced in recent releases and didn't see that until now. filed issue: googleapis/google-cloud-go#11001

My point was that we probably don't want to be getting these warnings, right? We want to quickstart to be able to run without warnings if the project has the correct permissions.

Failed to enable client metrics isn't logged with existing tests. However, current implementation is emitting a warning that the Resource Detector detected a different project compared to the project used by the Go client. Can talk through this with you when you're back on.

tritone · 2024-10-16T04:17:10Z

storage/storage_grpc_quickstart/main_test.go

+)
+
+const (
+	testPrefix      = "test-gcs-grpc-team"


Do we still need an allowlisted prefix now that this is GA? Let's just say storage-grpc-quickstart-test maybe?

Yes it's still required; fails otherwise.

Hmm when is the allowlist dropped?

Let's use the same prefix we use in the storage client integration tests ("golang-grpc-test").

Updated as it's not longer required.

tritone · 2024-10-16T04:19:15Z

storage/storage_grpc_quickstart/main.go

+	defer client.Close()
+
+	// Creates a Bucket instance.
+	bucket := client.Bucket(*bucketName)


This doesn't need to be a separate line of code and comment; can just do client.Bucket(*bucketName).Create() on L52 instead.

telpirion · 2024-10-15T20:22:55Z

storage/storage_grpc_quickstart/main.go

nit: let's move this sample to the storage_quickstart folder. I think it makes more sense to have these two quickstarts side-by-side.

FWIW, it looks like the Java and C++ versions of this sample are in the same folders & packages as the HTTP version.

C++

Java

Package names need to match if stored in the same directory which means we can't use main() for both. I need to put it into a different directory at least in Golang.

https://pkg.go.dev/golang.org/x/tools/internal/typesinternal#MismatchedPkgName

Moving into separate directory to unblock PR.

telpirion · 2024-10-15T20:26:50Z

storage/storage_grpc_quickstart/main.go

+	ctx := context.Background()
+
+	// Expects Google Cloud Platform project ID and Cloud Storage bucket
+	projectID := flag.String("project", "", "Cloud Platform project id")


issue: current best practice for samples is to forego a CLI.

See the Sample Style Guide:
https://googlecloudplatform.github.io/samples-style-guide/#no-cli

In the case of Go quickstarts, we've typically just not included tests (not great, I know) so as to have a "main" method.

telpirion · 2024-10-15T20:27:08Z

storage/storage_grpc_quickstart/main_test.go

@@ -0,0 +1,67 @@
+// Copyright 2024 Google LLC


See previous comment about testing quickstarts.

feat(storage): add gRPC quickstart sample

16d23e5

product-auto-label bot added api: storage Issues related to the Cloud Storage API. samples Issues that are directly related to samples. labels Oct 15, 2024

frankyn marked this pull request as ready for review October 15, 2024 17:27

frankyn requested review from a team as code owners October 15, 2024 17:27

telpirion self-assigned this Oct 15, 2024

tritone reviewed Oct 16, 2024

View reviewed changes

tritone assigned tritone and unassigned telpirion Oct 16, 2024

frankyn and others added 5 commits October 16, 2024 09:52

address initial feedback

11e4fa5

add check for failing metrics

f34d153

Merge branch 'main' into add_grpc_quickstart

58eaaf4

increase deadline

369a4fb

disable direct path

430251b

danielduhh added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 17, 2024

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 17, 2024

revert directpath disable

8776909

tritone added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 17, 2024

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 17, 2024

disable directpath

7f78077

frankyn added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 18, 2024

frankyn and others added 6 commits October 21, 2024 10:50

update bucket prefix

960fef7

Merge branch 'main' into add_grpc_quickstart

0195d2f

revert dp disablement

7c120ef

disable metrics

ac0df23

disable metrics in test clean up client

3e6d191

update test to not contain

4ff5678

increase timeout for testing..

8f46233

telpirion self-assigned this Oct 22, 2024

telpirion approved these changes Oct 22, 2024

View reviewed changes

frankyn and others added 3 commits October 22, 2024 15:11

no cli and no tests

334b8c4

adjust timeout to 10s

3ee6064

Merge branch 'main' into add_grpc_quickstart

ebe90e7

frankyn removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 22, 2024

tritone approved these changes Oct 22, 2024

View reviewed changes

frankyn enabled auto-merge (squash) October 22, 2024 22:18

move grpc quickstart into separate directory

36faf23

frankyn merged commit 73ef014 into main Oct 22, 2024
10 checks passed

frankyn deleted the add_grpc_quickstart branch October 22, 2024 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): add gRPC quickstart sample #4438

feat(storage): add gRPC quickstart sample #4438

frankyn commented Oct 15, 2024 •

edited

Loading

snippet-bot bot commented Oct 15, 2024 •

edited

Loading

tritone left a comment

tritone Oct 15, 2024

frankyn Oct 16, 2024

tritone Oct 16, 2024

frankyn Oct 21, 2024

tritone Oct 16, 2024

frankyn Oct 16, 2024

tritone Oct 16, 2024

frankyn Oct 21, 2024

tritone Oct 16, 2024

telpirion Oct 15, 2024

frankyn Oct 22, 2024

telpirion Oct 15, 2024

telpirion Oct 15, 2024


		stdOut, stdErr, err := m.Run(nil, time.Minute, "--project", tc.ProjectID, "--bucket", bucketName)

		if err != nil {

feat(storage): add gRPC quickstart sample #4438

feat(storage): add gRPC quickstart sample #4438

Conversation

frankyn commented Oct 15, 2024 • edited Loading

Description

Checklist

snippet-bot bot commented Oct 15, 2024 • edited Loading

tritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn commented Oct 15, 2024 •

edited

Loading

snippet-bot bot commented Oct 15, 2024 •

edited

Loading