refactor: update all callers to use new NewConfig API#306
Merged
Conversation
Contributor
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Mar 25, 2026 1:50p.m. | Review ↗ |
| Shell | |
Mar 25, 2026 1:50p.m. | Review ↗ |
meling
force-pushed
the
feature/298/simplify-newconfig-signature
branch
2 times, most recently
from
cad6f15 to
822cab8
Compare
Base automatically changed from
feature/298/simplify-newconfig-signature
to
master
March 25, 2026 13:44
Changes NewConfig(opts ...Option) to NewConfig(nodes NodeListOption, opts ...DialOption), making the required NodeListOption explicit and aligning with standard Go API conventions (required args first). Additional changes: - Add Configuration.Close() error for closing connections without accessing the internal Manager directly. - Add unexported Configuration.mgr() used by Extend and Add internally. - Deprecate Configuration.Manager() in favor of Configuration.Close(). - Deprecate NewConfiguration; callers should use NewConfig instead. - Remove Manager, NewManager, NewConfiguration from generated code template; update NewConfig wrapper to the new signature. - Remove Manager from the reservedIdents list. NOTE: Existing generated files still call the old gorums.NewConfig signature and will fail to compile until regenerated in commit 6. Closes #298. Related to #294.
Update non-generated callers to use the new NewConfig(nodes, opts...) API: - config.go: close manager on error in NewConfig to prevent resource leak - system.go: use cfg (Configuration) as io.Closer directly instead of cfg.Manager() - system_test.go: use cfg.Close() instead of cfg.Manager().Close() - testopts.go: rename existingMgr to existingCfg, update WithManager to accept Configuration instead of *Manager; update shouldSkipGoleak and type switch - testing_bufconn.go: use existingCfg.mgr() in getOrCreateManager - testing_integration.go: use existingCfg.mgr() in getOrCreateManager - config_test.go: replace NewManager+NewConfiguration with NewConfig; remove redundant mgr.Size() checks - server_test.go: use NewConfig in TestTCPReconnection - internal/tests/config/config_test.go: use WithManager(t, c1) instead of WithManager(t, c1.Manager()) Update template static code to remove Manager/NewManager/NewConfiguration wrappers and update NewConfig signature in generated code: - cmd/protoc-gen-gorums/dev/aliases.go: remove Manager type alias, NewManager and NewConfiguration functions; update NewConfig to take (nodes NodeListOption, opts ...DialOption) - cmd/protoc-gen-gorums/gengorums/template_static.go: regenerate from aliases.go
meling
force-pushed
the
feature/299/update-all-callers
branch
from
50924a6 to
8a55f2c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the codebase to consistently use the new NewConfig(nodes, dialOptions...) constructor API (introduced in #298), removing remaining internal/test call sites that relied on the older NewManager + NewConfiguration pattern.
Changes:
- Refactor tests and helpers to construct configurations via
NewConfig(...)and clean up viacfg.Close(). - Update test option plumbing to reuse an existing configuration’s underlying manager instead of passing a
*Manager. - Adjust system lifecycle management to register
Configurationitself as a closer (rather thancfg.Manager()).
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
config.go |
Updates NewConfig construction flow and ensures manager is closed on constructor error. |
config_test.go |
Migrates configuration-construction tests to NewConfig and updates teardown accordingly. |
server_test.go |
Refactors reconnection test to use NewConfig and closes the configuration in cleanup. |
system.go |
Registers the outbound Configuration itself as an io.Closer for system shutdown. |
system_test.go |
Updates cleanup to close configuration via cfg.Close() instead of cfg.Manager().Close(). |
testopts.go |
Changes WithManager test option to accept a Configuration for manager reuse. |
testing_integration.go |
Reuses manager from an existing configuration in integration build setup. |
testing_bufconn.go |
Reuses manager from an existing configuration in bufconn build setup. |
internal/tests/config/config_test.go |
Updates internal generated tests to reuse the manager via WithManager(t, cfg). |
cmd/protoc-gen-gorums/dev/aliases.go |
Aligns generated dev aliases wrapper NewConfig docs/signature usage with new API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Updates all internal callers, generated benchmark/example code, and tests to use the new
NewConfig(nodes, opts...)API introduced in #298.This removes all direct use of the old two-step
NewManager+NewConfigurationpattern throughout the codebase.Changes
benchmark/generated code to useNewConfigexamples/to useNewConfig*_gorums.pb.gofilesRelated
Part of #294
Closes #299