chore: adds fs access check at startup time #1030
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1030 +/- ##
==========================================
+ Coverage 82.72% 83.01% +0.29%
==========================================
Files 162 163 +1
Lines 9080 7636 -1444
==========================================
- Hits 7511 6339 -1172
+ Misses 1319 1044 -275
- Partials 250 253 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
internal/environment/checks.go
Outdated
| } | ||
| defer file.Close() | ||
|
|
||
| _, err = file.WriteString("check fs data") |
There was a problem hiding this comment.
Not sure this check is 100% needed as CreateTemp is creating a file and opening it for writing hence not sure about the need of actually writing in it, also if we need that I'd write a single byte as that seems to be enough cc @anuraaga
There was a problem hiding this comment.
Thanks, I took a better look at CreateTemp, and your thoughts are reasonable. I've removed it for now
internal/environment/checks.go
Outdated
|
|
||
| // IsDirWritable is a helper function to check if the WAF has access to the filesystem | ||
| func IsDirWritable(logger debuglog.Logger, dir string) error { | ||
| if !HasAccessToFS { |
There was a problem hiding this comment.
I'd rather decouple this from the other as we are mixing two concerns here. Also it is possible that for tinygo.wasm we could panic to make sure we don't call this when HasAccessToFS is false cc @anuraaga
There was a problem hiding this comment.
I split the function based on the build tag and wrapped it checking HasAccessToFS. PTAL
| // IsDirWritable is a helper function to check if the WAF has access to the filesystem | ||
| // It is unexpected to call this function when no_fs_access build tag is enabled | ||
| func IsDirWritable(logger debuglog.Logger, dir string) error { | ||
| panic("Unexpected call to IsDirWritable with no_fs_access build tag") |
|
Any input on this @anuraaga otherwise I think we can merge it. |
Fix #1007