Conversation
|
Looks quite good! The version change is the risky part. I don't know what will happen to monolith and composer when monolith asks for 2.x and a monolith dependency (she'll command) asks for 3.x. Does PHP and/or composer have a solution for this? Or did the way Amazon upgraded (by re using namespace) make it impossible? Def keep separate for now as lambda can use separate she'll command from monolith. But long term we will need a good solution. Nice work! |
|
I've updated the code. The s3 region is now hardcoded. Maybe we can talk about a better design at some point. |
| 'secret' => $this->s3SecretKey, | ||
| ], | ||
| 'region' => $s3Region, | ||
| 'region' => 'us-east-1', // hardcoded since Tourbuzz only uses this regions for S3 atm. |
There was a problem hiding this comment.
i think the right answer actually is to expose it as an option the same way that s3Key and s3SecretKey are used, and then make the hard-code in the client code that bootstraps the ShellCommandRunner.
However I don't really understand the use cases for region swapping, so maybe it's possible that the region would need to change on a per-ShellCommand basis? I am not sure, but I don't think that too many people juggle regions a ton, nor do too many people use ShellCommand. So I think the above approach (moving it into an option and letting client hard-code it) is a great fix for now that safely de-couples this from tourbuzz.
2 participants
@apinstein. Check these changes.
Not sure if this should be merged into master though. I mainly created this PR for you to review it.
I already pushed a tag version for the changes in this branch. Check it out!