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

refactor: convert RepoType from int to interface #8086

Merged
merged 10 commits into from
Mar 21, 2022

Conversation

nonsense
Copy link
Member

@nonsense nonsense commented Feb 14, 2022

Related Issues

This PR is convering RepoType type from int to interface, so that we can use the repo package in other projects and provide a custom Config, that is not referred to in Lotus (such as config.Boost).

The new usage can be seen at: filecoin-project/boost#231


We want to be using the filecoin-project/lotus/node/repo package, due to the fx dependency injection, as we want to integrate all Lotus markets dependencies directly in boost before releasing the alpha version.

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

@nonsense nonsense force-pushed the nonsense/refactor-nodetype branch 4 times, most recently from 78279c2 to 4df3c89 Compare February 14, 2022 11:44
@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #8086 (7cdf6dc) into master (a94e47c) will increase coverage by 0.29%.
The diff coverage is 49.61%.

❗ Current head 7cdf6dc differs from pull request most recent head 057c09b. Consider uploading reports for the commit 057c09b to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8086      +/-   ##
==========================================
+ Coverage   40.23%   40.53%   +0.29%     
==========================================
  Files         682      676       -6     
  Lines       74428    73575     -853     
==========================================
- Hits        29946    29821     -125     
+ Misses      39247    38537     -710     
+ Partials     5235     5217      -18     
Impacted Files Coverage Δ
cli/auth.go 0.00% <0.00%> (ø)
cmd/lotus-shed/datastore.go 0.00% <0.00%> (ø)
cmd/lotus-shed/rpc.go 0.00% <0.00%> (ø)
node/repo/fsrepo.go 38.91% <15.15%> (-4.86%) ⬇️
cli/util/api.go 26.78% <20.00%> (+2.51%) ⬆️
node/modules/storageminer.go 61.50% <60.48%> (-2.30%) ⬇️
node/repo/memrepo.go 69.03% <66.66%> (-0.47%) ⬇️
node/modules/storageminer_dagstore.go 70.00% <70.00%> (+8.46%) ⬆️
node/config/dynamic_config.go 75.00% <75.00%> (ø)
node/builder.go 76.92% <100.00%> (+0.22%) ⬆️
... and 79 more

@nonsense nonsense marked this pull request as ready for review February 14, 2022 11:55
@nonsense nonsense requested a review from a team as a code owner February 14, 2022 11:55
node/repo/memrepo.go Outdated Show resolved Hide resolved
node/repo/fsrepo.go Outdated Show resolved Hide resolved
node/repo/fsrepo.go Outdated Show resolved Hide resolved
node/modules/storageminer_dagstore.go Show resolved Hide resolved
@nonsense nonsense force-pushed the nonsense/refactor-nodetype branch 4 times, most recently from b2096e3 to 9fd7e71 Compare February 16, 2022 13:17
@nonsense
Copy link
Member Author

Thanks for review @magik6k !!! I think I addressed all your comments.

@magik6k magik6k self-requested a review February 17, 2022 19:37
@nonsense
Copy link
Member Author

@magik6k we modified a bit further the config in order to be able to pass in config.Boost. I am not happy with how we handle the dynamic configs... any suggestions on how to do that in a better way?

node/config/types.go Outdated Show resolved Hide resolved
node/repo/fsrepo.go Show resolved Hide resolved
node/builder.go Outdated Show resolved Hide resolved
cli/util/api.go Outdated Show resolved Hide resolved
@nonsense
Copy link
Member Author

nonsense commented Mar 7, 2022

@magik6k I think I addressed the latest comments.

@nonsense
Copy link
Member Author

@magik6k I rebased to master, and I think this is ready for merge. Could you have another look, and let me know if you think we need anything else as part of this PR?

@magik6k magik6k merged commit fc34d9b into master Mar 21, 2022
@magik6k magik6k deleted the nonsense/refactor-nodetype branch March 21, 2022 10:57
@nonsense
Copy link
Member Author

thanks!! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants