feat: lock w3up uploads feature switch open #2556
Conversation
commenting code out here because I'll make a cleanup pass to remove the switch entirely once this is stable in production
and update assertion in another
Deploying nft-storage with
|
| Latest commit: |
609514a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://045a82b1.nft-storage-1at.pages.dev |
| Branch Preview URL: | https://feat-w3up-uploads-enabled.nft-storage-1at.pages.dev |
| const client = await createClientWithUser(t) | ||
| const config = getTestServiceConfig(t) | ||
| const mf = getMiniflareContext(t) | ||
| // TODO verify with @alanshaw that we don't need to do this in the new upload flow |
There was a problem hiding this comment.
@alanshaw @vasco-santos could you weigh in on whether this dag-completeness check still needs to happen? it looks like we are still doing this check but maybe need to mock out upload/get in order to get this test passing? it looks like it's failing here because we have not mocked this out: https://github.com/nftstorage/nft.storage/blob/main/packages/api/src/routes/nfts-upload.js#L193
should we mock this out to get the test passing or just skip/remove this test?
There was a problem hiding this comment.
I would say no, but there is some impact that we need to take a more people decision
|
@alanshaw @vasco-santos - could you two take a look at the tests we're skipping here and verify that they seem like things we no longer need to test? @gobengo and I were just not highly confident and this feels like a good moment to double check that the things these tests are trying to verify are no longer parts of what this code is expected to do... |
| const client = await createClientWithUser(t) | ||
| const config = getTestServiceConfig(t) | ||
| const mf = getMiniflareContext(t) | ||
| // TODO verify with @alanshaw that we don't need to do this in the new upload flow |
There was a problem hiding this comment.
of course this means we should remove it also from the code
nft.storage/packages/api/src/routes/nfts-upload.js
Lines 190 to 206 in ad33faf
nft.storage/packages/api/src/routes/nfts-upload.js
Lines 220 to 225 in ad33faf
There was a problem hiding this comment.
yep! I'm planning on removing the whole legacy codepath plus these tests once this is running reliably in production
Co-authored-by: Benjamin Goering <171782+gobengo@users.noreply.github.com>
|
afaict this will be safe. I've been testing uploads from an account allowed by the feature switch, and haven't gotten any errors in prod. |
🤖 I have created a release *beep* *boop* --- ## [4.6.0](api-v4.5.0...api-v4.6.0) (2024-04-04) ### Features * added script to generate ucan for w3up space and docs for how to access the space using console.web3.storage ([#2554](#2554)) ([ad33faf](ad33faf)) * lock w3up uploads feature switch open ([#2556](#2556)) ([6eeefeb](6eeefeb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
commenting code out here because I'll make a cleanup pass to remove the switch entirely once this is stable in production