Repository navigation
[Go] Avoid importing gcpopts in gcsx and dataflowlib to prevent flag collisions - #40729
Conversation
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR cleanly resolves global flag collisions (--region and --project) caused by importing gcpopts in gcsx and dataflowlib. By relocating GetUniverseDomain and GetGcsEndpoint (using flag.Lookup) to gcsx while retaining delegating wrappers in gcpopts, the change breaks the problematic transitive dependency while preserving backward compatibility.
🔍 General Feedback
- The package dependency direction (
gcpopts->gcsx) and use offlag.Lookup("gcs_endpoint")effectively prevent unintended registration of global flags onflag.CommandLinefor consumers ofgcsxanddataflowlib. - Unit tests have been appropriately migrated and updated to cover both the core implementation in
gcsxand the delegating wrappers ingcpopts.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40729 +/- ##
===========================================
- Coverage 80.04% 56.42% -23.63%
===========================================
Files 490 872 +382
Lines 82916 154080 +71164
===========================================
+ Hits 66373 86937 +20564
- Misses 16543 65285 +48742
- Partials 0 1858 +1858
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Will fix CodeQL failure in another PR since its not related to this one. |
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
…_regionflagFix
|
Assigning reviewers: R: @jrmccluskey for label go. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Problem: Importing
gcpoptsingcsxanddataflowlib(#40686) transitively registersgcpopts's global--regionand--projectflags onflag.CommandLine, causingflag redefinedpanics in binaries that define their own--regionor--projectflags.Fix: This PR removes the
gcpoptsdependency fromgcsxanddataflowlibby:GetUniverseDomaininto a new flag-freeutil/gcpxpackage (sdks/go/pkg/beam/util/gcpx), whichgcsx,dataflowlib, and other GCP packages can import without registering global flags.GetGcsEndpointtogcsx(usingflag.Lookup("gcs_endpoint")instead of referencing*gcpopts.GcsEndpointdirectly).gcpoptsto preserve its public API.#40730