fix: add runtime version to user agent if present #1542

shubha-rajan · 2022-11-11T20:13:57Z

Add proxy operator version to user agent string if CSQL_PROXY_RUNTIME environment variable is set

Fixes GoogleCloudPlatform/cloud-sql-proxy-operator#67

hessjcg

LGTM

hessjcg · 2022-11-11T21:05:28Z

cmd/root.go

+func getUserAgentString() string {
+	userAgentString := "cloud-sql-proxy/" + versionString
+	operatorVersion, isSet := os.LookupEnv("CLOUD_SQL_PROXY_OPERATOR_VERSION")
+	if isSet {


if !isSet return?

enocom · 2022-11-14T16:16:09Z

cmd/root.go

+	userAgent = getUserAgentString()
+}
+
+func getUserAgentString() string {


Go prefers to leave off get for Getters. https://go.dev/doc/effective_go#Getters

enocom · 2022-11-14T16:16:47Z

cmd/root.go

+}
+
+func getUserAgentString() string {
+	userAgentString := "cloud-sql-proxy/" + versionString


Since we are inside a function about user agent, this could juse be ua, or u, or something similarly short.

enocom · 2022-11-14T16:18:05Z

cmd/root.go

+
+func getUserAgentString() string {
+	userAgentString := "cloud-sql-proxy/" + versionString
+	operatorVersion, isSet := os.LookupEnv("CLOUD_SQL_PROXY_OPERATOR_VERSION")


Same here -- a short variable is ok since it's clear what it is. Also, isSet is typically named ok.

enocom · 2022-11-14T16:18:31Z

cmd/root_test.go


+func Test_UserAgentWithOperatorVersion(t *testing.T) {
+	os.Setenv("CLOUD_SQL_PROXY_OPERATOR_VERSION", "0.0.1")
+    defer os.Unsetenv("CLOUD_SQL_PROXY_OPERATOR_VERSION")


This needs to be run through go-fmt.

enocom · 2022-11-14T16:18:42Z

cmd/root_test.go

+	os.Setenv("CLOUD_SQL_PROXY_OPERATOR_VERSION", "0.0.1")
+    defer os.Unsetenv("CLOUD_SQL_PROXY_OPERATOR_VERSION")
+
+	expected := "cloud-sql-proxy-operator/0.0.1"


want is conventional here

enocom · 2022-11-14T16:19:05Z

cmd/root_test.go

+    defer os.Unsetenv("CLOUD_SQL_PROXY_OPERATOR_VERSION")
+
+	expected := "cloud-sql-proxy-operator/0.0.1"
+	userAgentString := getUserAgentString()


got is a common name for variables here.

enocom · 2022-11-14T16:45:13Z

We should make this generic such that other runtimes can use it too.

See https://github.com/GoogleCloudPlatform/cloud-sql-jdbc-socket-factory/blob/acb57cb9ce803062651a4b4764e9181237d84a23/core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java#L439-L454 where we already do this in the Java connector.

enocom · 2022-11-16T19:17:57Z

cmd/root.go

+	if !ok {
+		return ua
+	}
+	rv, ok := os.LookupEnv("CLOUD_SQL_PROXY_RUNTIME_VERSION")


Shall we just use CLOUD_SQL_PROXY_RUNTIME and let the runtime include version?

enocom · 2022-11-16T19:19:24Z

cmd/root_test.go

 	return c, err
 }

+func Test_UserAgentWithOperatorVersion(t *testing.T) {


We've been omitting underscores in test names. Let's remove the underscore here.

cmd/root_test.go

enocom · 2022-11-16T22:08:47Z

cmd/root.go

+
+func userAgentString() string {
+	ua := "cloud-sql-proxy/" + versionString
+	runtime, ok := os.LookupEnv("CLOUD_SQL_PROXY_RUNTIME")


@kurtisvg pointed out that some runtimes (e.g., Cloud run where they'll use this programatically) might not be able to set an environment variable.

Let's make this a CLI flag proper (which will also support an environment variable).

I'm in favor of a single flag rather than a repeatable flag.

enocom · 2022-11-17T01:24:09Z

cmd/root.go


 	// Global-only flags
+	pflags.StringVar(&c.runtime, "runtime", "",
+		"Runtime and version, e.g. cloud-sql-proxy-operator/0.0.1")


Let's add something like "(for internal use only)".

This is a port of GoogleCloudPlatform/cloud-sql-proxy#1542 and GoogleCloudPlatform/cloud-sql-proxy#1561.

shubha-rajan added 2 commits November 11, 2022 12:11

fix: add k8s operator version to user agent if present

b61b903

test that userAgent is properly set

7b516ec

shubha-rajan marked this pull request as ready for review November 11, 2022 21:03

shubha-rajan requested a review from a team November 11, 2022 21:03

hessjcg approved these changes Nov 11, 2022

View reviewed changes

hessjcg reviewed Nov 11, 2022

View reviewed changes

update getUserAgentString function

fa17d88

enocom requested changes Nov 14, 2022

View reviewed changes

shubha-rajan added 3 commits November 16, 2022 10:34

address review comments

6f6605f

make environment variable generic

88be7aa

Merge branch 'main' into operator-version-user-agent

cdc1c4d

shubha-rajan requested a review from enocom November 16, 2022 19:07

enocom reviewed Nov 16, 2022

View reviewed changes

shubha-rajan changed the title ~~fix: add k8s operator version to user agent if present~~ fix: add runtime version to user agent if present Nov 16, 2022

blunderbuss-gcf bot assigned enocom Nov 16, 2022

shubha-rajan added 2 commits November 16, 2022 11:27

use one variable for setting runtime user agent

c7706e9

rename test

e01e62c

enocom approved these changes Nov 16, 2022

View reviewed changes

Merge branch 'main' into operator-version-user-agent

c01511e

enocom self-requested a review November 16, 2022 21:38

enocom reviewed Nov 16, 2022

View reviewed changes

shubha-rajan added 2 commits November 16, 2022 16:45

make runtime a CLI flag

4a0a707

update tests

c3da678

shubha-rajan force-pushed the operator-version-user-agent branch from e483e4f to 52a15ec Compare November 17, 2022 01:10

linting

72b62eb

shubha-rajan force-pushed the operator-version-user-agent branch from 52a15ec to 72b62eb Compare November 17, 2022 01:11

enocom requested changes Nov 17, 2022

View reviewed changes

add (for internal use only) comment

16e6fc1

shubha-rajan requested a review from enocom November 17, 2022 01:33

enocom approved these changes Nov 17, 2022

View reviewed changes

shubha-rajan merged commit a6b689b into main Nov 17, 2022

shubha-rajan deleted the operator-version-user-agent branch November 17, 2022 22:15

release-please bot mentioned this pull request Nov 17, 2022

chore(main): release 2.0.0-preview.4 #1552

Merged

enocom added a commit to GoogleCloudPlatform/alloydb-auth-proxy that referenced this pull request Jan 15, 2023

feat: add support for user-agent configuration

78e2618

This is a port of GoogleCloudPlatform/cloud-sql-proxy#1542 and GoogleCloudPlatform/cloud-sql-proxy#1561.

enocom added a commit to GoogleCloudPlatform/alloydb-auth-proxy that referenced this pull request Jan 18, 2023

feat: add support for user-agent configuration

b0ebc61

This is a port of GoogleCloudPlatform/cloud-sql-proxy#1542 and GoogleCloudPlatform/cloud-sql-proxy#1561.

enocom mentioned this pull request Jan 18, 2023

feat: add support for user-agent configuration GoogleCloudPlatform/alloydb-auth-proxy#232

Merged

enocom added a commit to GoogleCloudPlatform/alloydb-auth-proxy that referenced this pull request Jan 18, 2023

feat: add support for user-agent configuration (#232)

0b1d3e9

This is a port of GoogleCloudPlatform/cloud-sql-proxy#1542 and GoogleCloudPlatform/cloud-sql-proxy#1561.

fix: add runtime version to user agent if present #1542

fix: add runtime version to user agent if present #1542

Uh oh!

Conversation

shubha-rajan commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hessjcg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enocom commented Nov 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shubha-rajan commented Nov 11, 2022 •

edited

Loading