Conversation
The integration tests are quite heavyweight, and running them 10 times in parallel (i.e. JSON + msgpack across 5 Go versions) adds unnecessary load to the sandbox cluster. Go has a backwards compatibility guarantee between versions, and also only officially supports the two latest Go versions, so it's sufficient for us to only test on the latest two Go versions in CI. Signed-off-by: Lewis Marshall <lewis.marshall@ably.com>
WalkthroughThe CI workflows for Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/check.yml (1)
19-20: Consider aligning Go version requirements with tested versions.The
go.moddeclaresgo 1.18, but CI only tests Go 1.25 and 1.26. This creates a gap where regressions on Go 1.18–1.24 won't be caught. While Go's backwards compatibility likely makes this acceptable, either:
- Update
go.modtogo 1.25if 1.18 support is no longer needed- Add a note to README or CONTRIBUTING explaining that only 1.25+ is tested, even though the module may theoretically support 1.18+
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/check.yml around lines 19 - 20, The CI tests run on the workflow variable go-version (values ['1.25','1.26']) but go.mod currently declares "go 1.18", creating a mismatch; either update the go directive in go.mod from 1.18 to 1.25 (edit the go directive in go.mod) so module requirement matches tested versions, or add a clear note in README or CONTRIBUTING indicating that CI only tests Go 1.25+ while the module may still target 1.18; pick one approach and ensure the go-version list in the workflow and the go directive (or the documentation note) are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/check.yml:
- Around line 19-20: The CI tests run on the workflow variable go-version
(values ['1.25','1.26']) but go.mod currently declares "go 1.18", creating a
mismatch; either update the go directive in go.mod from 1.18 to 1.25 (edit the
go directive in go.mod) so module requirement matches tested versions, or add a
clear note in README or CONTRIBUTING indicating that CI only tests Go 1.25+
while the module may still target 1.18; pick one approach and ensure the
go-version list in the workflow and the go directive (or the documentation note)
are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 144644c0-2405-43fd-95d7-140f4e5bc2c7
📒 Files selected for processing (2)
.github/workflows/check.yml.github/workflows/integration-test.yml
The integration tests are quite heavyweight, and running them 10 times in parallel (i.e. JSON + msgpack across 5 Go versions) adds unnecessary load to the sandbox cluster.
Go has a backwards compatibility guarantee between versions, and also only officially supports the two latest Go versions, so it's sufficient for us to only test on the latest two Go versions in CI.
Summary by CodeRabbit