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

Remove storedask from provider #263

Merged
merged 1 commit into from
May 20, 2020
Merged

Conversation

hannahhoward
Copy link
Collaborator

Goals

Add one more element of datastore customization

Implementation

  • Move stored ask out of provider so its setup and configured seperately.
  • Also allow customizing the DS key for stored ask
  • Remove prefix for provider in DS store
  • Modify integration tests

@codecov-commenter
Copy link

Codecov Report

Merging #263 into master will increase coverage by 0.98%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
+ Coverage   64.06%   65.03%   +0.98%     
==========================================
  Files          39       39              
  Lines        2245     2299      +54     
==========================================
+ Hits         1438     1495      +57     
+ Misses        690      687       -3     
  Partials      117      117              
Impacted Files Coverage Δ
storagemarket/impl/provider.go 5.47% <0.00%> (+0.07%) ⬆️
storagemarket/impl/storedask/storedask.go 80.52% <66.67%> (ø)
storagemarket/impl/providerstates/provider_fsm.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f56af3...3906e9e. Read the comment docs.

Copy link
Contributor

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

Nothing blocking; LGTM

@@ -220,6 +222,8 @@ func newStorageHarness(ctx context.Context, t *testing.T) *storageHarness {
)
require.NoError(t, err)
dt2 := graphsyncimpl.NewGraphSyncDataTransfer(td.Host2, td.GraphSync2, td.DTStoredCounter2)
storedAsk, err := storedask.NewStoredAsk(td.Ds2, datastore.NewKey("latest-ask"), providerNode, providerAddr)
assert.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a nitpick but I think this should be a require.NoError since the test doesn't make sense unless this succeeds.

Move stored ask out of provider so its setup and configured seperately. Also allow customizing the
DS key
@hannahhoward hannahhoward force-pushed the feat/remove-stored-ask branch from 3906e9e to 7d68bbc Compare May 20, 2020 21:28
@hannahhoward hannahhoward merged commit 2f01d9e into master May 20, 2020
@dirkmc dirkmc mentioned this pull request Oct 5, 2021
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.

3 participants