-
Notifications
You must be signed in to change notification settings - Fork 111
fix: Remove fetch_remote_manifest feature
#1618
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
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #1618 will not alter performanceComparing Summary
Footnotes
|
|
How does this impact the cli tools? |
|
@tmathern I preserved the same behavior for c2patool. This PR only changes the SDK. |
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.
Why is it a verify.remote_manifest_fetch settings? Verify implies a validation for me? But this is more a "read", in the sense we allow to get remote data.
What will be the expectations for the downstream SDKs? They all get the "remote manifest fetch" turned off by default and need to set the appropriate setting if they want it back?
Yes I agree, we should consider changing this, but we probably want to preserve backwards compatibility as well.
Yes this will change that functionality. I suggest we wait to merge this until we can pass settings to readers/builders individually. |
Sidenote: We are already breaking backwards compatibility with that change (builds will break, but I agree it's different than also breaking settings files... but it seems everyone who wants that functionality to stay will need to update settings anyway). |
Removes the
fetch_remote_manifestfeature and changes theverify.remote_manifest_fetchsetting to default to false. This is related to avoiding network requests by default in the SDK. Note this is NOT a backwards compatible change although effectively has the same behavior as if the feature wasn't enabled.