GoogleCloudPlatform / golang-samples

Sample apps and code written for Google Cloud in the Go programming language.
Apache License 2.0
4.29k stars 1.73k forks source link

fix: Fix environment variable substitution for GOOGLE_CLOUD_PROJECT. #4345

Closed coderkakarrot closed 2 weeks ago

coderkakarrot commented 1 month ago

Description

Fixes #4346

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

Checklist

dashpole commented 1 month ago

@aabmass

aabmass commented 1 month ago

@subfuzion I've seen a few PRs that I thought would fix the broken test

/tmpfs/src/github/golang-samples/opentelemetry/instrumentation/app/go.mod:3: invalid go version '1.21.0': must match format 1.23

any idea what's wrong here?

subfuzion commented 1 month ago

@subfuzion I've seen a few PRs that I thought would fix the broken test


/tmpfs/src/github/golang-samples/opentelemetry/instrumentation/app/go.mod:3: invalid go version '1.21.0': must match format 1.23

any idea what's wrong here?

I do know what the issue is. I can help explain the problem in a few minutes, but I'm away from my desk right now.

subfuzion commented 1 month ago

@subfuzion I've seen a few PRs that I thought would fix the broken test


/tmpfs/src/github/golang-samples/opentelemetry/instrumentation/app/go.mod:3: invalid go version '1.21.0': must match format 1.23

any idea what's wrong here?

I do know what the issue is. I can help explain the problem in a few minutes, but I'm away from my desk right now.

Beginning with Go 1.21, go mod tidy updates go.mod with Go version in x.y.z format. Our CI builds currently tests with Go 1.20 as the earliest version, which expects x.y format.

This mismatch creates a conflict because our lint/vet action currently uses 1.21 (and thus fail after after detecting a change after running go mod tidy if using the x.y format) or else fail in the Kokoro build on Go 1.20 if using the x.y.z format.

Here's what should work for now

To update a sample to Go 1.21, run go get go@1.21 in the package directory Run go get toolchain@none (or just delete the line as part of the next step) Manually edit go.mod to change go 1.21.XX to ONLY go 1.21 -- this is required so that the CI build for the current earliest version (1.20) doesn't fail (x.y.z format won't work for 1.20). This issue will go away when we upgrade the earliest version to 1.21.

subfuzion commented 1 month ago

@subfuzion I've seen a few PRs that I thought would fix the broken test


/tmpfs/src/github/golang-samples/opentelemetry/instrumentation/app/go.mod:3: invalid go version '1.21.0': must match format 1.23

any idea what's wrong here?

I do know what the issue is. I can help explain the problem in a few minutes, but I'm away from my desk right now.

Beginning with Go 1.21, go mod tidy updates go.mod with Go version in x.y.z format. Our CI builds currently tests with Go 1.20 as the earliest version, which expects x.y format.

This mismatch creates a conflict because our lint/vet action currently uses 1.21 (and thus fail after after detecting a change after running go mod tidy if using the x.y format) or else fail in the Kokoro build on Go 1.20 if using the x.y.z format.

Here's what should work for now

To update a sample to Go 1.21, run go get go@1.21 in the package directory Run go get toolchain@none (or just delete the line as part of the next step) Manually edit go.mod to change go 1.21.XX to ONLY go 1.21 -- this is required so that the CI build for the current earliest version (1.20) doesn't fail (x.y.z format won't work for 1.20). This issue will go away when we upgrade the earliest version to 1.21.

I just confirmed that this doesn't help since the GitHub workflow runs go mod tidy using v1.21), which updates go.mod to use the x.y.z numbering -- this result leaves an uncommitted updated go.mod, which fails lint. If instead we use 1.21.0, this will fail in the Kokoro build, which uses go 1.20.

I'm going to have to discuss resolution with the team and reply here later.

aabmass commented 4 weeks ago

@subfuzion and update here or can I help get this change in?

telpirion commented 2 weeks ago

Let's see if we can get this small PR green & merged!

aabmass commented 2 weeks ago

@telpirion any ideas how to get this CI passing? It's been stuck in CI limbo for a few weeks now.

Or, could we just force merge this since the change doesn't have any coverage (yet)?

telpirion commented 2 weeks ago

@telpirion any ideas how to get this CI passing? It's been stuck in CI limbo for a few weeks now.

Or, could we just force merge this since the change doesn't have any coverage (yet)?

@aabmass Yeah, I'm probably going to just force merge this PR. I'm getting tired of chasing down phantom CI errors :/.

aabmass commented 2 weeks ago

Ty, appreciate all the help here