feat: Inject ACP instance into the DB instance #2633
Conversation
AndrewSisley
added
feature
AndrewSisley
force-pushed
the
2632-acp-to-new-db
branch
from
d04b17b
to
09d3237
Compare
|
question: Is acp in mem and non-mem usage now dependent on the path config? |
As far as I could see it was always so, the todo in |
node/node.go
Outdated
| @@ -112,7 +121,7 @@ func NewNode(ctx context.Context, opts ...NodeOpt) (*Node, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| db, err := db.NewDB(ctx, rootstore, options.dbOpts...) | |||
| db, err := db.NewDB(ctx, rootstore, acp.NoACP, options.dbOpts...) | |||
There was a problem hiding this comment.
question: Why not also dependent on the default options like for rootstore above?
For example:
acp, err := NewACP(options.acpOpts...)
if err != nil {
return nil, err
}
// Then use:
db, err := db.NewDB(ctx, rootstore, acp, options.dbOpts...)There was a problem hiding this comment.
Because I managed to forget to actually hook it up after doing all the rest :)
- Actually use the lovely new code you wrote
| func WithACPType(acpType ACPType) ACPOpt { | ||
| return func(o *ACPOptions) { | ||
| o.acpType = acpType | ||
| } | ||
| } | ||
|
|
||
| // WithACPPath sets the ACP path. | ||
| // | ||
| // Note: An empty path will result in an in-memory ACP instance. | ||
| func WithACPPath(path string) ACPOpt { | ||
| return func(o *ACPOptions) { | ||
| o.path = path | ||
| } | ||
| } | ||
|
|
||
| // NewACP returns a new ACP module with the given options. | ||
| func NewACP(ctx context.Context, opts ...ACPOpt) (immutable.Option[acp.ACP], error) { |
There was a problem hiding this comment.
suggestion: Can preserve / modify the tests that were removed in internal/db/config_test.go to test the public functions in this file. Perhaps in a node/acp_test.go file or something?
There was a problem hiding this comment.
I would much rather spend time fixing the integration tests to cover this properly than to add unit tests here. #2634
There was a problem hiding this comment.
Thanks for making the issue. I do believe the tests that were removed were very recently added, I forget why that extra effort was put and if there was a reason for them.
AndrewSisley
force-pushed
the
2632-acp-to-new-db
branch
from
2ce2156
to
66631fe
Compare
The ACP interface is public public, and will be returned from node.New shortly, as well as several node option funcs.
The ACP instance is now exposed/injected in a similar fashion to that of the rootstore.
Annoyingly I added it exactly and only for this reason and somehow forgot/got distracted from actually using it...
AndrewSisley
force-pushed
the
2632-acp-to-new-db
branch
from
66631fe
to
bc10e70
Compare
Relevant issue(s)
Resolves #2632
Description
Injects the ACP instance into the DB instance, like how rootstore is provided.
Also makes the
acppackage public, and moves the ACP options off ofdband intonode(similar to rootstore).Prerequisite for #2366 (as well as being something I think we inherently wanted).