feat(internal/librarian/python): first part of bump for python#3938
feat(internal/librarian/python): first part of bump for python#3938jskeet wants to merge 3 commits intogoogleapis:mainfrom
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 version bumping functionality for Python libraries by updating gapic_version.py files. While the implementation is well-structured and includes comprehensive unit tests, it contains a reliability issue (potential panic on directory access errors) and a security vulnerability due to insecure handling of symbolic links. These issues, particularly related to error handling, should be addressed by adding proper error checking and ensuring that only regular files are processed during directory traversal.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3938 +/- ##
==========================================
- Coverage 83.35% 83.32% -0.03%
==========================================
Files 69 69
Lines 6188 6190 +2
==========================================
Hits 5158 5158
- Misses 671 673 +2
Partials 359 359 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adds version bumping for gapic_version.py files. There will be other files (in other formats) to bump over time, but this gets us a bit closer.
| // we don't expect version numbers to require quoting). Any existing text after | ||
| // "__version__ = " (on the same line) is omitted. All other lines are | ||
| // preserved. | ||
| func bumpGapicVersions(output, version string) error { |
There was a problem hiding this comment.
It looks like Bump and bumpGapicVersions have the exact same function signature. Can we just inline bumpGapicVersions?
There was a problem hiding this comment.
There will definitely be additional files to bump. My intention is to keep Bump really simple, just calling one function after another. I figured I might as well start with the structure I want to use later.
internal/librarian/python/bump.go
Outdated
| ) | ||
|
|
||
| const gapicVersionLinePrefix = "__version__ = " | ||
| const gapicVersionFile = "gapic_version.py" |
There was a problem hiding this comment.
nit:
const (
gapicVersionLinePrefix = "version = "
apicVersionFile = "gapic_version.py"
)
| // preserved. | ||
| func bumpGapicVersions(output, version string) error { | ||
| return filepath.WalkDir(output, func(path string, d fs.DirEntry, err error) error { | ||
| if d.Name() == gapicVersionFile { |
There was a problem hiding this comment.
Could you change to
if d.Name() != gapicVersionFile {
return nil
}See https://github.com/googleapis/librarian/blob/main/doc/howwewritego.md#avoid-unnecessary-else
3 participants
Adds version bumping for gapic_version.py files. There will be other files (in other formats) to bump over time, but this gets us a bit closer.