johannesboyne / gofakes3

A simple fake AWS S3 object storage (used for local test-runs against AWS S3 APIs)
MIT License
361 stars 84 forks source link

Auto bucket creation option #65

Closed shabbyrobe closed 2 years ago

shabbyrobe commented 2 years ago

This adds a new option to gofakes3: WithAutoBucket, which is off by default. The idea is that instead of failing when a bucket doesn't exist, it gets automatically created before first use. The use case I have is supplying a fake s3 instance as part of a wider suite of development helpers for an environment with a lot of services that expect the existence of lots of buckets. Being able to just presume any bucket we try to use is valid saves a fairly substantial setup step. I have a couple of other minor changes that relate to this workflow, with a view to supplying a more useful Dockerfile in a later PR than the one I chucked in there a few years ago.

I've added the "ensure bucket" check to each operation that needs to trigger the autobucket logic, but it raises another interesting conundrum: we don't really try to emulate any of the atomicity guarantees that S3 has. This change potentially makes that problem worse by expanding the surface of TOCTOU that we have (I haven't thought this through fully yet, just calling out that it's on my mind), but I think some sort of opt-in atomicity support might be something I could try to tackle later?

codecov-commenter commented 2 years ago

Codecov Report

Merging #65 (a52390b) into master (532d036) will decrease coverage by 0.67%. The diff coverage is 16.66%.

:exclamation: Current head a52390b differs from pull request most recent head 25f5452. Consider uploading reports for the commit 25f5452 to get more accurate results

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
- Coverage   68.04%   67.36%   -0.68%     
==========================================
  Files          28       28              
  Lines        2497     2534      +37     
==========================================
+ Hits         1699     1707       +8     
- Misses        563      581      +18     
- Partials      235      246      +11     
Impacted Files Coverage Δ
error.go 76.13% <ø> (ø)
log.go 80.00% <0.00%> (-20.00%) :arrow_down:
gofakes3.go 64.39% <14.63%> (-3.65%) :arrow_down:
option.go 72.72% <100.00%> (+2.72%) :arrow_up:
uploader.go 88.51% <0.00%> (+0.95%) :arrow_up:
backend/s3mem/bucket.go 83.21% <0.00%> (+3.64%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 532d036...25f5452. Read the comment docs.

johannesboyne commented 2 years ago

@shabbyrobe it just looks like one of the tests is now failing, even though without a lot of information.

shabbyrobe commented 2 years ago

Sorry @johannesboyne, adding you to the PR wasn't intended to be a nudge, it was more meant to say "hey I think I'm actually ready now" 😆

I'll fix that test right away.

johannesboyne commented 2 years ago

Haha ;-)! I for sure would have needed the nudging!

sonarcloud[bot] commented 2 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

shabbyrobe commented 2 years ago

Test is fixed! I think the codecov diff is getting tanked a bit by adding more "assumer" tests; we don't run the assumer in the build (and I don't reckon we should - it's part of the reason why go's test framework isn't used, to avoid it getting picked up by someone running go test ./...). Is it ok to merge as is?

johannesboyne commented 2 years ago

Agree, it's just confused. Let's merge.