-
Notifications
You must be signed in to change notification settings - Fork 1
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: Send download intent to peers #944
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, just a few small things. I definitely prefer the name "download intent".
Are there any tests we could add for this?
|
||
/** | ||
* | ||
* @param {object} opts | ||
* @param {import('../core-manager/index.js').CoreManager} opts.coreManager | ||
* @param {CoreOwnership} opts.coreOwnership | ||
* @param {import('../roles.js').Roles} opts.roles | ||
* @param {import('../types.js').BlobFilter | null} opts.blobDownloadFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we always set this to null
. Can we either (1) set the option (2) remove the option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is waiting for #940 to land so we can use the NON_ARCHIVE_DEVICE_FILTER constant for initializing the class. Left it out to avoid creating a dependency on that PR for landing this.
We could add temporary tests, which dive into the implementation details and list to events in the core manager, or we can wait until we update sync wants based on the download intents which will enable us to write end-to-end tests. I'm tempted to just wait for e2e tests. |
Fixes #686