Enables snapshot external storage provider by default #1798
Conversation
| export async function getConfiguredAttachmentStoreConfigs(): Promise<IAttachmentStoreConfig[]> { | ||
| if (GRIST_EXTERNAL_ATTACHMENTS_MODE === 'snapshots') { | ||
| const snapshotProvider = create.getAttachmentStoreOptions().snapshots; | ||
| // This shouldn't happen - it could only happen if a version of Grist removes the snapshot provider from ICreate. |
There was a problem hiding this comment.
Is this comment now out of date?
There was a problem hiding this comment.
Still valid here, just a very unlikely edge case. This could only happen if someone makes a custom fork of Grist / removes the snapshot provider support entirely.
There was a problem hiding this comment.
Wouldn't it be reasonable to continue to throw at the site immediately below then? If it really should never happen? I'm confused then.
There was a problem hiding this comment.
50/50 - I think either would make sense here.
The overall change here is moving from "Error when snapshots aren't available but requested" to "Do nothing when snapshots are requested but not available".
This could easily throw here given it's unlikely to ever come up, and if it is, it's indicative of a code issue rather than a config one.
I defaulted I think mostly to keep the semantics here consistent across all config values (e.g. GRIST_EXTERNAL_ATTACHMENTS_MODE="gobbledygook" won't error).
I'm happy with either - error when ATTACHMENTS_MODE is set to something the Grist version doesn't support, or quietly ignore it. What do you think?
There was a problem hiding this comment.
Throwing for things that shouldn't happen has saved us many times when they do, so that has my vote.
The desired behavior in this change is, if a user makes a grist installation and configures external storage, external attachments become available using that store (if there's no specific configuration to counter that). It throwing here is consistent with that outcome, I vote for keeping it.
| // This shouldn't happen - it could only happen if a version of Grist removes the snapshot provider from ICreate. | ||
| if (snapshotProvider === undefined) { | ||
| throw new Error("Snapshot provider is not available on this version of Grist"); | ||
| return []; |
There was a problem hiding this comment.
Not here, but somewhere it feels like a log message along the lines of "no snapshot storage configuration found, external attachments also disabled" would be useful.
There was a problem hiding this comment.
I agree, but couldn't think where the most sensible place would be - it's kind of here?
This is where the config value is translated into a usable object for the rest of the system, and validity checks happen. It's pretty much the only part of the system that's aware of "User has requested snapshots, snapshots not available".
We could, effectively, memoize this function, so it only fires a log entry the first time it's called perhaps?
There was a problem hiding this comment.
Yes, having one log entry appear, at this site, through whatever means, would be good. Thanks Spoffy.
Context
Currently no external storage is enabled by default. This hinders both discovery of the feature, and getting it "put through its paces" by the wider Grist community.
Proposed solution
This enables external attachments in 'snapshot' mode by default. If a Grist installation has an external storage configured (e.g. S3, MinIO) for snapshots, it will also enable that to be used for attachments.
Two checks were changed from throwing startup errors to silent defaults. The checks made sense under the assumption that Grist has been misconfigured if 'snapshots' external storage was specified, and no storage configured / available.
Has this been tested?