Skip to content
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

Move S3Client to the container. #72

Merged

Conversation

lukeraymonddowning
Copy link
Contributor

Howdy!

So, here's the deal. Currently, both LambdaClient and CloudWatchLogsClient are bound in the Laravel container, which is wonderful, because that means if we need to override that binding in user-land, we can. 😈

However, in Hammerstone\Sidecar\Package.php, the S3Client is currently being manually instantiated, which means that

  1. It doesn't share the same paradigm as the other two services
  2. It can't be overridden by an end user (without some seriously crazy composer hacks)

This PR aims to fix that with the following steps:

  1. We place AWS' S3Client behind a Hammerstone S3Client that extends the former
  2. We bind that new S3Client in the SidecarServiceProvider
  3. We update Package to make use of this bound S3Client rather than manually instantiating

Whilst I was at it, I realised that what I actually tend to want as a user is to return a custom configuration array for all three services: Lambda, CloudWatch and S3 (for example, if I'm using token auth as an added layer of security). To help make it simple for a user to provide a custom config array for these services, I've made the following adjustments/additions:

  1. Created a new contract: AwsClientConfiguration, which expects implementations to return a config array
  2. Created a default implementation which simply performs the logic that was found in SidecarServiceProvider::getAwsClientConfiguration previously
  3. Bound that default implementation and used it for all three services in the container

There is a case I think for also passing the client FQCN as a parameter to the AwsClientConfiguration::getConfiguration method, which would allow for passing slightly different configurations depending on the service being used, but I opted not to add that in just yet pending your opinion.

Any how, any questions let me know!

Kind Regards,
Luke

Moves `S3Client` to a bound service in the container and adds a new `AwsClientConfiguration` that can be overridden to make user-land customisation easier.
Copy link
Owner

@aarondfrancis aarondfrancis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukeraymonddowning this is a great change, I'm super on board with it.

There is a case I think for also passing the client FQCN as a parameter to the AwsClientConfiguration::getConfiguration method, which would allow for passing slightly different configurations depending on the service being used, but I opted not to add that in just yet pending your opinion.

Is this something y'all need at the moment? If not, I may opt for holding off for now. The only reason I could think to include it immediately is that it's a public function and I think changing the signature in the future is technically a breaking change. But since we're still in the 0 series it's not a huge deal. I'd say up to you, depending on if you need it now or not.

Let me know what you decide and I can merge + tag.

Thanks again!

@lukeraymonddowning
Copy link
Contributor Author

@aarondfrancis we don't need that, so let's leave it as is for now 👍

@aarondfrancis aarondfrancis merged commit 627d157 into aarondfrancis:main May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants