Handle Git commit timestamp for pseudo-version migration#190
Conversation
WalkthroughAdds version-preservation to dependency migration by passing current module versions into MigrateDependencies, adjusts DoMigration to record existing versions and run go mod before/after dependency updates, and improves pseudo-version generation with robust GitHub API handling and commit-time selection. Tests updated to reflect new flow and additional command invocations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as migrate (CLI)
participant Mig as DoMigration
participant FS as Filesystem
participant Go as go mod
User->>CLI: Run migration
CLI->>Mig: DoMigration(cwd, target)
alt Not skipping Go modules
Mig->>FS: dependencyVersions(cwd)
FS-->>Mig: origDeps map
Mig->>Go: run go mod
Mig->>Mig: execute eligible migrations (excl. deps)
Mig->>Mig: MigrateDependencies(cmd, cwd, origDeps, target)
Mig->>Go: run go mod
else Skipping Go modules
Mig->>Mig: execute eligible migrations
end
Mig-->>CLI: Done
CLI-->>User: Result
sequenceDiagram
autonumber
participant CLI as pseudoVersionFromHash
participant GH as GitHub API
participant JSON as JSON Decoder
CLI->>GH: GET /repos/.../commits/{sha} with User-Agent
GH-->>CLI: HTTP response
alt status != 200
CLI->>CLI: read body (fallback to status text)
CLI-->>CLI: error with status and body
else status == 200
CLI->>JSON: decode commit Author/Committer dates
alt Committer.Date present
CLI->>CLI: commitTime = Committer.Date
else Author.Date present
CLI->>CLI: commitTime = Author.Date
else none
CLI-->>CLI: error (no commit time)
end
CLI->>CLI: normalize UTC, truncate seconds
CLI-->>CLI: construct pseudo-version with commitTime
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refines the dependency migration process and pseudo-version generation logic within the CLI tool. It ensures that existing higher dependency versions are preserved during migration, normalizes Git commit timestamps to UTC seconds for accurate pseudo-version derivation, and correctly updates go.mod files by rerunning module commands after dependency adjustments.
Highlights
- Dependency Version Preservation: The migration logic now explicitly tracks and preserves higher dependency versions already declared in go.mod files, preventing unintended downgrades.
- Accurate Pseudo-Version Timestamps: Git commit timestamps used for generating pseudo-versions are now normalized to UTC and truncated to the second, improving consistency and accuracy.
- Robust Go Module Updates: The migration process includes additional steps to re-run go mod commands, ensuring go.mod files are consistently updated after dependency changes.
- Improved API Request Handling: Enhanced error handling for HTTP requests made to fetch commit information, including setting a User-Agent and checking for non-200 status codes.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to the migration process. It correctly handles Git commit timestamps for pseudo-version generation by normalizing them to UTC and considering both author and committer dates. It also enhances dependency management by preserving higher existing dependency versions during migration and ensures go.mod is kept up-to-date by running go mod commands at appropriate times.
The changes are well-implemented and include corresponding test updates. I have one suggestion to improve error handling when reading the response body from the GitHub API to make the tool more robust.
| msg, err := io.ReadAll(res.Body) | ||
| if err != nil || len(msg) == 0 { | ||
| msg = []byte(res.Status) | ||
| } |
There was a problem hiding this comment.
The error returned by io.ReadAll is being swallowed. If reading the response body fails, the error is ignored. This can mask underlying issues like network problems. It's better to handle this error explicitly and return it.
msg, err := io.ReadAll(res.Body)
if err != nil {
return "", fmt.Errorf("http request failed with status %s, and could not read response body: %w", res.Status, err)
}
if len(msg) == 0 {
msg = []byte(res.Status)
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
cmd/migrate.go (4)
141-142: Good: Add UA; consider Accept + optional auth to avoid 403/rate-limits.Setting a User-Agent is required by GitHub. To improve reliability under rate limits and to future-proof content negotiation, consider also sending Accept and an Authorization header when GITHUB_TOKEN is present.
Apply:
req.Header.Set("User-Agent", "fiber-cli") +req.Header.Set("Accept", "application/vnd.github+json") +if token := os.Getenv("GITHUB_TOKEN"); token != "" { + req.Header.Set("Authorization", "Bearer "+token) +}
143-143: Bound the HTTP client's total time.The context timeout cancels the request, but a client-level Timeout protects DNS/connect-time as well. This hardens the call.
-client := http.Client{} +client := http.Client{Timeout: 5 * time.Second}
153-159: Limit error-body size to avoid pathological memory use.GitHub error bodies are small, but reading unbounded data is unnecessary. Cap to ~4KB.
- msg, err := io.ReadAll(res.Body) + lr := &io.LimitedReader{R: res.Body, N: 4096} + msg, err := io.ReadAll(lr)
176-187: Robust commit time selection and normalization.Fallback from committer to author and UTC second-truncation are correct for pseudo-versions. Minor improvement: include the hash in the error for easier debugging.
- return "", errors.New("commit date not found") + return "", fmt.Errorf("commit date not found for commit %s", hash)cmd/migrate_test.go (1)
286-286: Expectation updated to 6 external calls is correct.DoMigration now runs go mod twice (tidy/download/vendor each), so 2×3 = 6. Consider also asserting the exact commands to guard ordering regressions, but not required for this PR.
cmd/internal/migrations/dependencies.go (2)
82-84: Nil-safe read from current map.Indexing a nil map is safe; this is fine. Consider documenting that current can be nil for clarity.
122-149: Helper to snapshot dependency versions is sound; consider small nits.
- The implementation mirrors the parsing in MigrateDependencies; extracting a tiny helper to parse a go.mod's require block could remove duplication.
- Add a brief doc comment describing the returned structure: map[moduleDir]map[importPath]*semver.Version.
+// dependencyVersions returns, for each module directory under root, +// a map of required module import paths to their parsed semantic versions. +// It ignores replace/exclude directives by design. func dependencyVersions(root string) (map[string]map[string]*semver.Version, error) {cmd/internal/migrations/lists.go (1)
147-152: Running go mod before and after dependency bump is reasonable; optional micro-opt.Two passes keep go.mod/vendor consistent. If you want to avoid the second pass when nothing changed, consider returning a boolean from MigrateDependencies indicating whether any module was updated.
- if err := MigrateDependencies(cmd, cwd, origDeps, target); err != nil { + changed, err := MigrateDependencies(cmd, cwd, origDeps, target) // signature change + if err != nil { errs = append(errs, fmt.Errorf("migrate dependencies: %w", err)) } - if err := internal.RunGoMod(cwd); err != nil { + if changed { + if err := internal.RunGoMod(cwd); err != nil { errs = append(errs, fmt.Errorf("go mod: %w", err)) + } + }Note: only if you're comfortable adjusting the signature; not required for this PR.
cmd/internal/migrations/dependencies_test.go (1)
96-103: No-change path validated well.Ensures idempotency when project already meets/exceeds required versions. Consider adding a test where the project uses a pseudo-version higher than Fiber's requirement to cover that path too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
cmd/internal/migrations/dependencies.go(3 hunks)cmd/internal/migrations/dependencies_test.go(2 hunks)cmd/internal/migrations/lists.go(3 hunks)cmd/migrate.go(3 hunks)cmd/migrate_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/dependencies_test.go (1)
cmd/internal/migrations/dependencies.go (1)
MigrateDependencies(26-120)
cmd/internal/migrations/lists.go (3)
cmd/internal/migrations/common.go (1)
MigrateGoPkgs(21-58)cmd/internal/migrations/dependencies.go (1)
MigrateDependencies(26-120)cmd/internal/go_mod.go (1)
RunGoMod(20-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-13)
🔇 Additional comments (10)
cmd/migrate.go (3)
6-6: Imports look correct and purposeful."errors" is used for a specific sentinel error and "io" is required for response body handling. No issues.
Also applies to: 8-8
161-171: Struct shape matches GitHub; pointer dates are the right choice.Accepting nil dates for committer/author aligns with API variability. Looks good.
195-197: Correct use of module.PseudoVersion.Supplying major path "vN", base tag, UTC time, and 12-char short SHA conforms to Go's pseudo-version rules. Trimming leading "v" for semver parsing upstream is consistent.
cmd/internal/migrations/dependencies.go (4)
21-26: Doc clarifies behavior; good emphasis on preserving higher versions.The updated comment accurately states the new contract and the meaning of the current map.
89-93: maxVer logic correctly avoids downgrades.Preferring the greater of Fiber's requirement and the project's prior version fulfills the PR goal.
97-99: Version bump condition is precise.Using LessThan(maxVer) ensures only needed upgrades. Parsing handles pseudo/pre-release forms used by Go.
26-26: AllMigrateDependenciescall sites updated
Verified that every invocation now uses the new four-argument signature (cmd, cwd, current, target):
- cmd/internal/migrations/lists.go: line 147
- cmd/internal/migrations/dependencies_test.go: lines 55 and 102
No lingering old-arity calls remain.
cmd/internal/migrations/lists.go (2)
33-33: Decoupling dependency bump from early migrations is cleaner.Leaving the first migration to package path/version edits only reduces coupling and surprises.
97-104: Recording original versions before mutations enables correct maxVer decisions.Capturing origDeps pre-migration matches the intention to preserve project-declared higher versions.
cmd/internal/migrations/dependencies_test.go (1)
49-56: Tests cover version preservation across modules.Seeding curr per-dir verifies that fasthttp upgrades and brotli stays pinned. Clear and aligned with behavior.
1 participant
Summary
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68ab17e410a48326a4439c5787689f4d
Summary by CodeRabbit
New Features
Bug Fixes