Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(node-builder): depinject db pruners and manager #1394

Merged
merged 7 commits into from
Jun 9, 2024

Conversation

ocnc2
Copy link
Contributor

@ocnc2 ocnc2 commented Jun 9, 2024

i love generics

Copy link
Contributor

@archbear archbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits otherwise lgtm


// AvailabilityPrunerInput is the input for the ProviderAvailabilityPruner
// function for the depinject framework.
type AvailabilityPrunerInput struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: AvailabilityStorePruner

mod/node-core/pkg/components/availability_pruner.go Outdated Show resolved Hide resolved
mod/node-core/pkg/components/defaults.go Outdated Show resolved Hide resolved
// Pruner is a struct that holds the prunable interface and a notifier channel.
type Pruner[
// DBPruner is a struct that holds the prunable interface and a notifier channel.
type DBPruner[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can make this a private type with a public constructor

Copy link
Contributor Author

@ocnc2 ocnc2 Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we still need to reference the public type though to make the injection into dbmanager work

@ocnc2 ocnc2 marked this pull request as ready for review June 9, 2024 01:49
Copy link

codecov bot commented Jun 9, 2024

Codecov Report

Attention: Patch coverage is 2.43902% with 80 lines in your changes missing coverage. Please review.

Project coverage is 21.53%. Comparing base (91f40e2) to head (bbef4f0).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1394      +/-   ##
==========================================
- Coverage   21.53%   21.53%   -0.01%     
==========================================
  Files         236      238       +2     
  Lines       11238    11239       +1     
  Branches       18       18              
==========================================
  Hits         2420     2420              
- Misses       8656     8657       +1     
  Partials      162      162              
Files Coverage Δ
mod/storage/pkg/manager/manager.go 70.58% <ø> (ø)
mod/storage/pkg/pruner/pruner.go 71.42% <100.00%> (ø)
mod/node-core/pkg/components/runtime.go 0.00% <0.00%> (ø)
mod/node-core/pkg/components/block_feed.go 0.00% <0.00%> (ø)
mod/node-core/pkg/components/module/depinject.go 0.00% <0.00%> (ø)
mod/node-core/pkg/builder/builder.go 0.00% <0.00%> (ø)
mod/node-core/pkg/components/defaults.go 0.00% <0.00%> (ø)
mod/node-core/pkg/components/db_manager.go 0.00% <0.00%> (ø)
mod/node-core/pkg/components/deposit_store.go 0.00% <0.00%> (ø)
mod/node-core/pkg/components/availability_store.go 0.00% <0.00%> (ø)

@ocnc2 ocnc2 requested a review from archbear June 9, 2024 05:18
Signed-off-by: ocnc2 <169075746+ocnc2@users.noreply.github.com>
@itsdevbear itsdevbear added this pull request to the merge queue Jun 9, 2024
@itsdevbear
Copy link
Contributor

itsdevbear commented Jun 9, 2024

@ocnc2 imma merge, but address archs feedback after

Merged via the queue into main with commit b167dc0 Jun 9, 2024
15 checks passed
@itsdevbear itsdevbear deleted the depinject-db-stuff branch June 9, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants