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

add CloudFormation Custom Resource #6

Merged
merged 5 commits into from
Oct 20, 2021
Merged

add CloudFormation Custom Resource #6

merged 5 commits into from
Oct 20, 2021

Conversation

bpholt
Copy link
Member

@bpholt bpholt commented Sep 1, 2021

I updated one of our CloudFormation custom resources to use this draft implementation: Dwolla/postgresql-init-custom-resource#6

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks!! Really excited about this. Still need to take a closer look but just added some quick thoughts.

build.sbt Outdated Show resolved Hide resolved
import io.circe.syntax._
import io.circe.generic.semiauto._

package object cloudformation {
Copy link
Member

Choose a reason for hiding this comment

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

Can some of this stuff be moved to the events module maybe? Perhaps a bit over-modulization, but I think its valuable to offer our case class encodings of the basic AWS event types so people can use them with their own custom IOLambdas without buying into our implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went back and forth on using the events module. I see where you're coming from by allowing others to use the models, but it also means as a user of just the CloudFormation Custom Resource class, I have a lot of models in scope that actually aren't relevant to that task. Tradeoffs either way, I think.

Copy link
Member

@armanbilge armanbilge Sep 9, 2021

Choose a reason for hiding this comment

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

I agree, certainly tradeoffs either way, definitely open to ideas for how to best tackle this. FWIW the events module is akin to aws-lambda-java-events and aws-lambda/trigger. Maybe we could scope this a bit better with additional packages?

Also, to explain my thinking a bit, I tend to determine modules by their dependencies. Taking the JS implementations as an example:

  • core: CE3
  • lambda: core + circe
  • lambda-events: circe-generic (i.e. shapeless :)
  • lambdaApiGatewayProxyHttp4s: lambda + events + http4s-core
  • lambdaCloudFormationCustomResource: lambda + events + http4s-ember-client

So, the events module essentially lets users write lambdas without having http4s (or even CE3!) in scope if that's not relevant to what they want to do either.

I think it ultimately comes down to priorities, if we pretty much assume people will be doing things our way then I totally agree with you, no need for an events module and we can keep things scoped.

Copy link
Member Author

@bpholt bpholt Oct 20, 2021

Choose a reason for hiding this comment

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

My preference is to leave these things in the CloudFormation module, so I left this alone for now. I think if people aren't doing things our way they're more likely to just use aws-lambda-java-events or whatever, and if they are using our core then having a more focused set of model classes makes sense. But either way is fine, so if there's consensus to move them into events, I can do that too.

@armanbilge
Copy link
Member

armanbilge commented Sep 9, 2021

@bpholt Apologies I wasn't so clear in my comments above, I think I'm imagining an API like this without CfnSetup, hopefully this makes a bit more sense?

abstract class CloudFormationCustomResourceHandler[Input: Decoder, Output: Encoder]
  extends IOLambda[CloudFormationCustomResourceRequest[Input], Unit] {
  
  // Users can override if they wish
  def client: Resource[IO, Client[IO]] = EmberClientBuilder.default[IO].build
  
  // client is the only shared setup, users can setup anything else they want in this resource
  def handler(client: Client[IO]): Resource[CloudFormationCustomResource[IO, Input, Output]
  
  // Now we combine the above to get everything we need in Setup
  type Setup = (Client[IO], CloudFormationCustomResource[IO, Input, Output])
  override final def setup: Resource[IO, Setup] = for {
    c <- client
    h <- handler(c)
  } yield (c, h)
  // Ninja-ed the above to fix stupidity (:

  // ...
}

@bpholt bpholt marked this pull request as ready for review October 20, 2021 19:28
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks for finishing this up! Just some nits, but nothing to block on :)

build.sbt Outdated Show resolved Hide resolved
@armanbilge armanbilge merged commit 5d1cc91 into typelevel:main Oct 20, 2021
@armanbilge
Copy link
Member

@bpholt would it be helpful to publish a snapshot with this?

@bpholt bpholt deleted the cloudformation-custom-resource branch October 20, 2021 22:15
@bpholt
Copy link
Member Author

bpholt commented Oct 20, 2021

Yes please, a snapshot would be great

@armanbilge
Copy link
Member

Sorry, just saw this. Do you have keys you can install that can publish under org.typelevel? Otherwise I can publish under my own group.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants