Conversation
daniel-garcia
force-pushed
the
go1.20
branch
3 times, most recently
from
2d60606 to
09da5a7
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR is a work-in-progress refactoring to make the hotload library compatible with Go 1.20 while decoupling Prometheus dependencies from the core module. The changes involve significant architectural improvements including a provider pattern for metrics, removal of external dependencies, and migration from ginkgo/gomega to standard library testing.
Key changes:
- Downgrade from Go 1.23 to Go 1.20 and remove external dependencies (google/uuid, pkg/errors, teivah/onecontext)
- Introduce metrics provider abstraction layer to decouple Prometheus from core library
- Create separate
prometheusmetricssubmodule for optional Prometheus support - Replace custom implementations:
internal/randstringfor UUID,internal/contextutilfor context merging - Migrate tests from ginkgo/gomega to standard library testing framework
Reviewed Changes
Copilot reviewed 41 out of 45 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Downgrades to Go 1.20, removes all external dependencies except fsnotify |
| metrics/provider.go | Introduces provider interface for pluggable metrics implementations |
| prometheusmetrics/* | New submodule providing optional Prometheus metrics support |
| internal/randstring/randstring.go | Custom random string generator replacing google/uuid |
| internal/contextutil/contextutil.go | Custom context merger replacing teivah/onecontext |
| fsnotify/errors_compat.go | Custom error comparison (unnecessary - Go 1.20 has errors.Is) |
| conn.go | Replaces onecontext.Merge with contextutil.Merge |
| transaction.go | Updates to use new metrics provider abstraction |
| *_test.go | Migrates from ginkgo/gomega to standard library testing |
| Makefile | Updates test commands to include unit_tests build tag |
| Dockerfile.* | Updates Go version from 1.23 to 1.24 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| c.incExecStmtsCounter() //increment the exec counter to keep track of the number of exec calls | ||
| c.logf("managedConn.ExecContext", "calling underlying conn.ExecContext()") | ||
| mergedCtx, cancel := onecontext.Merge(c.ctx, ctx) | ||
| mergedCtx, cancel := contextutil.Merge(c.ctx, ctx) | ||
| defer cancel() | ||
| return conn.ExecContext(mergedCtx, query, args) | ||
| } |
There was a problem hiding this comment.
[nitpick] The change from onecontext.Merge to contextutil.Merge is good for removing external dependencies. However, the QueryContext method (line 187 in the file) still doesn't merge contexts like ExecContext does, creating an asymmetry in context handling that could lead to inconsistent behavior between query and exec operations.
daniel-garcia
force-pushed
the
go1.20
branch
from
09da5a7 to
8378963
Compare
daniel-garcia
force-pushed
the
go1.20
branch
from
8378963 to
1a83393
Compare
1 participant
No description provided.