brabster / crucible

AWS CloudFormation templates built with Clojure
Eclipse Public License 1.0
72 stars 18 forks source link

add support for AWS::Neptune:* resources #171

Closed shooit closed 5 years ago

shooit commented 5 years ago

I'm not opposed to putting them all in a different namespace. For these resource types there was a significant amount of overlap in the keys of each resource with no different specs so I thought that one namespace was more DRY.

This certainly breaks down when different resources have the same key with a different spec like you mention. It might be better to set that as a standard so future cases can be covered without a breaking change.

I think I've talked myself into breaking this up, will update the PR...

shooit commented 5 years ago

I settled on keeping properties/keys that were shared among the resources in the neptune.clj file and then referencing their specs in each neptune/<resource>.clj file.

Let me know what you think.

keerts commented 5 years ago

I settled on keeping properties/keys that were shared among the resources in the neptune.clj file and then referencing their specs in each neptune/<resource>.clj file.

Great, that's exactly what I thought to do when introducing this pattern. I somehow never ran into reusable specs for the resources I added or I didn't pay attention. But yeah.. great!

I kept the defresources together in the service namespace though (neptune.clj in your case). What do you think?

brabster commented 5 years ago

Good work both of you - I'll leave you to decide between you what 's best :+1: - mention me if you need any help merging/releasing @keerts

shooit commented 5 years ago

Putting the defresouces all in the neptune.clj file would create a circular dependency between neptune.clj and the neptune/<resource>.clj files, because they all reference the shared specs in neptune.clj and neptune.clj would then have to require all of the neptune/<resources>.clj files. I could put the shared specs in neptune/shared.clj but I don't really like that as much as it is now.

The potential issue I see with how this PR currently organized it that it could lead to a ballooning of the number of namespaces (one Clojure namespace per AWS resource type) if adopted across the project. I think think one to one mapping is a simple and understandable idiom to follow, but refactoring the current project to follow it would induce breaking changes.

I don't think there is any extra "work" put on the end-user if the code were organized this way. If specs are already defined across sub-namespaces (e.g. neptune.db-instance), the user will still have to require them all to generate templates, so it wouldn't require any additional LOC to have defresource defined in the sub-namespace as well.

shooit commented 5 years ago

The other option would be to redefine shared specs in each neptune/<resource>.clj file which would mean no circular dependencies, but then there is repeated code 🤷‍♂️. I don't know how often (if at all) AWS is changing or adding to the CF resource properties so maybe its not an issue to define a spec multiple times if it won't ever change.

keerts commented 5 years ago

Good observations. There are a couple of strategies now in the code and I feel like reorganizing it at some point to make it easier for template writers. Your PR could be the model for it and some parts are close (ecs / ecr / etc.) That would mean breaking changes and while they are bad, I dislike inconsistencies even more.

keerts commented 5 years ago

As a general comment and out of curiousity, why do you (and me for that matter ;-) ) put a lot of energy in this while a thing as Terraform exists? Terrraform shines in completeness, especially in the AWS area, while for crucible we have to create the resource definitions here first, before being able to build our template. On top of that, Cloudformation is not always the best experience you can hope for, I found Terraform to be failing faster and thus saving me a lot time.

brabster commented 5 years ago

:scream: you can't jump to Terraform!

More seriously, I know what you mean... my reasons for choosing CloudFormation were not having to manage the state files, having pretty reliable quality from AWS as new resources appeared and having pretty good rollback capabilities built in.

I wrote a Clojure wrapper because JSON was horrible to work with (pre-yaml days) and the existing library, called Condensation I think, was very macro-y and I found it really hard to work with. It also didn't do the schema thing so didn't help remember what resources went where.

These changes look good to me - any objections to merging them up and releasing?

shooit commented 5 years ago

I work for a Clojure shop that uses AWS for all of our deployments (and almost everything else). We had written (before Crucible was a thing) some bespoke tooling that would validate and deploy serverless topologies (Lambda/Kinesis). We decided as a company to switch over to using Crucible because it had more coverage than our own tooling and the spec validation and extensibility is great.

CloudFormation definitely isn't perfect, but with Crucible and the Cognitect AWS API we're getting to a place where all of our ops work can be done in clojure which is a huge win for us.

brabster commented 5 years ago

I should be home in couple of hours, I'll merge and release later unless I hear any objections.

brabster commented 5 years ago

If you're interested @shooit, happy to give you rights to merge and release. Your work is fantastic and it sounds like a faster turnaround might be useful. Let me know if you want that

shooit commented 5 years ago

@brabster I would really appreciate merge and release rights. I'm enjoying helping with this project!

brabster commented 5 years ago

I think you should have rights now - let me know if not. There's https://github.com/brabster/crucible/blob/master/CONTRIBUTING.md that hopefully describes how to perform a release but shout up if it's not clear and we'll improve it.