-
Notifications
You must be signed in to change notification settings - Fork 3
feat: allow overriding withCDN via env-var #55
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
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.
self review
if we were going to add this. 2 TODOs needs to be considered before this is merged is:
|
@jennijuju I could add a warning in here during |
The first version looks great to me! I would like filecoin-pin to print the FilBeam retrieval URL when CDN is enabled, but that can be added later. I did not realise there are implications for |
opened up todos: #61 and have a warning with > npm run build && WITH_CDN=true npx filecoin-pin add ~/Downloads/js-colo-media/71713366785__2CBE4B4C-4404-470F-AEAD-DFF56D011409.png
> filecoin-pin@0.6.0 build
> tsc
┌ Filecoin Pin Add
│
▲ CDN Pricing Notice
│
│
│ Filecoin-pin currently does not support CDN pricing in payment calculations.
│
│ This means:
│ • Deposit calculations may not be accurate for CDN storage
│ • You may need additional USDFC deposits for CDN-enabled uploads
│ • Filcdn is transitioning to egress-based billing (from fixed fees)
│
│
◇ Do you want to proceed with CDN-enabled upload?
│ No
└ Add cancelled
Add failed: CDN pricing limitations warning cancelled and for car import: > filecoin-pin@0.6.0 build
> tsc
┌ Filecoin Pin CAR Import
│
▲ CDN Pricing Notice
│
│
│ Filecoin-pin currently does not support CDN pricing in payment calculations.
│
│ This means:
│ • Deposit calculations may not be accurate for CDN storage
│ • You may need additional USDFC deposits for CDN-enabled uploads
│ • Filcdn is transitioning to egress-based billing (from fixed fees)
│
│
◇ Do you want to proceed with CDN-enabled upload?
│ No
└ Import cancelled
Import failed: CDN pricing limitations warning cancelled |
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.
I think this environment variable option for now is fine. It unblocks being able to test Filecoin Beam with filecoin-pin data.
What we really need though is a followup issue about adding "withCdn" support in general. This means exposing i through the CLI as an config option, spitting out filecoinbeam URLs, etc. plus handling the payment issues highlighted in #61.
I will get that issue created later today.
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.
Awesome, thank you @SgtPooki! ❤️
Quick mention in here (without looking at this), that |
I see I'm kind of duplicating Jen's comment about pricing already .. so my drive-by probably isn't very helpful here! |
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.
ok, I don't love the optionality in envToBool but whatever
Co-authored-by: Rod Vagg <rod@vagg.org>
needs a rebase and can be landed, I reckon @bajtos et. al. are going to like at least having the option on the table to start experimenting with this soon |
Removed in |
I ran this new version today, and encountered two problems:
|
oh, this is an ethers cleanup thing from Websockets, I thought we had this caught and ignored cause it'll be random and it doesn't indicate a problem |
Attempting more cleanup here, there's some async jobs that do later cleanup inside ethers that we're not catching. I think this will do it: #144 |
Uh oh!
There was an error while loading. Please reload this page.