Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adds initial support for external attachments using the Filesystem or MinIO #1320
base: main
Are you sure you want to change the base?
Adds initial support for external attachments using the Filesystem or MinIO #1320
Changes from 19 commits
2473375
ff2fe69
fc35744
ef13def
d19ff54
8295f5e
8f09573
72f2e5c
5e26ec8
20a1a79
482b5c7
59d1633
c1058da
74102d0
6640133
85ba845
3902f82
4085c06
65bc7a9
ce5282f
e53f6eb
819e0d6
52b7e24
79396dc
d527e59
c3c82bd
dc19e54
4d7dc11
340c5b3
6af1818
8ca7196
2d59def
fc9db0c
72eecf3
cdcfb0d
0bddba2
b9d2f8c
e5b508c
f712ef8
364ac08
ae78bba
4730d99
c39806f
58a101d
785971f
f93948f
a4768a5
c41b4db
f0222ca
3008fdd
4a1c112
adac94b
3b0f603
2ba5848
635a92c
dc55180
ae87fa1
b46b320
e44aadf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If there are no settings, should that be an error? In what circumstances might that happen?
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.
Ah, I see the method gets used in a migration. That answers that. Might still make sense to throw an error, but accept that error as a non-erroneous possibility in the migration. Feels like in all other circs it should be an error? No big deal.
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 can switch this to throwing an error, then wrap the code below in
_makeEngine
in a try-catch block to handle this case?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.
that'd be fine
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.
Say more? Are there useful things in your head that you don't need future devs to try to guess at about the role and purpose of attachment file managers?
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.
So all new logging ideally has a decent amount of structured context. Don't think "strings on a console", think "json in elasticsearch". That docName there embedded in a string will be useless for any aggregations.
This method is a better:
grist-core/app/server/lib/HostedStorageManager.ts
Line 132 in 76f1769
and this is gold standard so far, lots of context:
grist-core/app/server/lib/ActiveDoc.ts
Line 243 in 76f1769
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 a really valuable perspective switch for me. Thanks, will re-work these.
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.
Switched all the logging to include relevant metadata (doc names, store IDs, etc), and use LogMethods where appropriate