fix: add logic and cmd to update Transports in api.go#3940
fix: add logic and cmd to update Transports in api.go#3940
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new command and associated logic to update transport information in internal/serviceconfig/api.go by parsing BUILD.bazel files. The approach of using Go's AST manipulation capabilities is sound. However, I've identified a critical issue that causes the generation of non-compilable code due to incorrect assumptions about available language constants. Additionally, there's a minor bug in the transport simplification logic and a test that doesn't adhere to the project's testing style guide. My review includes suggestions to address these points.
zhumin8
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3940 +/- ##
==========================================
- Coverage 83.35% 83.25% -0.10%
==========================================
Files 69 70 +1
Lines 6188 6325 +137
==========================================
+ Hits 5158 5266 +108
- Misses 671 688 +17
- Partials 359 371 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // limitations under the License. | ||
|
|
||
| // Package importconfig provides commands for importing configurations. | ||
| package importconfig |
There was a problem hiding this comment.
Why do we need a command to do this? If this is part of the migration, we should write code in tool/cmd/migrate.
| return fmt.Errorf("failed to parse %s: %w", apiGoPath, err) | ||
| } | ||
|
|
||
| apisSlice := findAPIsSlice(astFile) |
There was a problem hiding this comment.
Can you write a comments for this function?
| } | ||
| transports := readTransports(googleapisDir, path) | ||
| if len(transports) == 0 || (len(transports) == 1 && transports["all"] == "grpc+rest") { | ||
| if transportsIdx != -1 { |
There was a problem hiding this comment.
What does this line mean?
| return | ||
| } | ||
| if diff := cmp.Diff(test.want, got); diff != "" { | ||
| if diff := cmp.Diff(test.want, got, cmpopts.IgnoreFields(API{}, "Transports")); diff != "" { |
There was a problem hiding this comment.
Could you add a comment on why we need to ignore transport?
|
|
||
| func TestUpdateTransportsCommand(t *testing.T) { | ||
| cmd := updateTransportsCommand() | ||
| ctx := context.Background() |
There was a problem hiding this comment.
2 participants
Add logic and command to loop through API list in api.go and update transport info based BUILD.bazel from googleapis.
This is intended to be used during migration, so for simplicity passing path to googleapis dir to command (we assume transport info does not change other than new libs).
For #3775