capybara-screenshot
capybara-screenshot copied to clipboard
mattheworiordan
Google Cloud Storage uploader
Thanks! I did write it after S3 implementation.
Before we jump into testing I wonder how scalable this approach is. I mean what if we have more some XYZ uploaders. We keep adding members to store corresponding configs in hashes and allowing only a single uploader to be used at a time. While, we may say it doesn't make sense to use 2 uploaders at the same time (why would anybody need that?). I wonder if we should have a single hash with :s3 and :gcs keys on top level and make it possible to use all of them if one will? I feel like some refactoring would be necessary though...
Now I realise the S3 driver has no test
I do see a unit test for S3. I can reuse that for GCS as well.
Also I don't think end-to-end is going to be hard if we are up for it. Ingress is free and if we delete uploads, I don't think we would have to pay anything for storage. With high availability and low latency it can be done. We would want to encrypt credentials though.
I'm going to rebase my stuff meanwhile as S3 path reporting changes have been merged... it will look clean for GCS as well.
@mlt sorry for the slow reply.
Initial thoughts:
- I don't think you should overengineer it at this stage. If we add another target bucket provider, then I think at that time we should consider a refactor. Unless of course you have the time and fancy doing that work. What I am saying is that I don't think it's a blocker to this useful feature.
- I don't believe an end-to-end test is necessary. What is necessary is a test that stubs out the GCS gem methods and makes sure they are called with the expected values. At least then the contract between this gem and that gem are tested, and we can rely on the GCS gem to keep their contracts with Google working.
WDYT?
uhm… no worries on slow replies…it took me over 2 month to get back to it 😆 I think it passes some tests now. I used S3 implementation as a reference, but I had to heavily modify it. I hope it tests most if not all cases. I do not see coverage reports.
i wonder why this is not implemented using the callbacks. would be much cleaner, not clutter the base class with specifics. could also be released as a seperate gem.
