feat: Use repometadata.GenerateRepoMetadata for generating .repo-metadata.json#3792
feat: Use repometadata.GenerateRepoMetadata for generating .repo-metadata.json#3792jameslynnwu wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the generation of .repo-metadata.json for Rust libraries to use a common repometadata.Generate function, which is a good step towards consolidating logic. However, the changes introduce a few critical issues that need to be addressed. There's a potential race condition due to incorrect handling of a loop variable in a goroutine, and a copy-paste error has led to the wrong repository name being used for Rust metadata generation. Additionally, related tests have not been updated to reflect these changes correctly. My review includes suggestions to fix these problems.
I am having trouble creating individual review comments. Click here to see my feedback.
internal/librarian/generate.go (127-131)
This change introduces a potential race condition. The loop variable lib is captured by the goroutine's closure. If the project's Go version is less than 1.22, all goroutines will likely operate on the last value of lib from the loop, as they will capture the variable by reference. To fix this, you should re-introduce the lib := lib statement to create a new variable for each iteration that can be safely captured by the closure.
for _, lib := range libraries {
lib := lib // Capture range variable.
g.Go(func() error {
return generate(gctx, cfg, lib, googleapisDir, rustSources)
})
}References
- As per the repository style guide, we should follow idiomatic Go patterns. The Go Code Review Comments wiki page warns against capturing loop variables in closures without creating a per-iteration copy, as it can lead to subtle bugs. See the 'Goroutine Lifetimes' section for more details. (link)
internal/librarian/rust/generate.go (83)
This appears to be a copy-paste error. The repository name for Rust metadata generation is hardcoded to the Python repository, "googleapis/google-cloud-python". This should be using the repository name from the configuration, which is available via cfg.Repo.
if err := repometadata.Generate(library, cfg.Language, cfg.Repo, absoluteServiceConfig, defaultVersion, library.Output); err != nil {
References
- Reuse existing functions for similar logic, such as formatting, to ensure consistency and maintainability. Using a configurable value (
cfg.Repo) instead of a hardcoded string improves maintainability and consistency across different language generators, aligning with this principle.
internal/librarian/python/generate.go (76)
The repository name is hardcoded as "googleapis/google-cloud-python". While this might be correct for Python, it's better to use the Repo field from the config.Config struct (cfg.Repo) to avoid hardcoding and improve maintainability. The cfg object is already available in this function.
if err := repometadata.Generate(library, cfg.Language, cfg.Repo, absoluteServiceConfig, defaultVersion, outdir); err != nil {
References
- Reuse existing functions for similar logic, such as formatting, to ensure consistency and maintainability. Using a configurable value (
cfg.Repo) instead of a hardcoded string improves maintainability and consistency across different language generators, aligning with this principle.
internal/librarian/rust/generate_test.go (257)
The test passes an empty config.Config{} to the Generate function. This will cause cfg.Language and cfg.Repo to be empty strings when calling repometadata.Generate, leading to an incorrectly generated .repo-metadata.json file.
Please update the test to pass a realistic configuration. You should also extend the test to verify the contents of the generated .repo-metadata.json file to ensure the new functionality is working as expected.
if err := Generate(t.Context(), &config.Config{Language: "rust", Repo: "googleapis/google-cloud-rust"}, library, sources); err != nil {
…s and remove the corresponding mustache template.
3b80f7c to
bc85670
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3792 +/- ##
==========================================
- Coverage 82.67% 82.43% -0.24%
==========================================
Files 139 139
Lines 12685 12798 +113
==========================================
+ Hits 10487 10550 +63
- Misses 1714 1760 +46
- Partials 484 488 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@jameslynnwu looks like this PR has been inactive for a while. Can we close it out for now to optimize for keeping the dashboard clean? We can always reopen it later if needed. |
Deletes the repo-metadata.json mustache template.
Fixes #3606