Add localstack as addon package#257
Conversation
Signed-off-by: Bradon Kanyid (rattboi) <rattboi@gmail.com>
30db402 to
b6f425a
Compare
|
Thanks for the changes @rattboi ... this looks good to me. I also played around with the code in your previous PR and landed on something similar but with its full functionality working. Take a look here: nimakaviani/idpbuilder/commit/07e92225124ff03a1af113fb94b0cb54b4ea1c1e Assuming that we also a few considerations:
Anyhow, please take a look and tell me what you think, and we can pull in whatever you like into this PR and merge it. thanks again for the very interesting line of work. |
|
I took a look at your branch, and it is interesting. I think personally I am not a fan of the fact that there is no composability with the overlay you have described, instead doing full swap-outs/replacements of the underlying manifests. I can see that it does work, but it also means that if the base is updated, you need to propagate those changes into whatever addon overlay you have as well. Also, if there were some other addon that needed coredns changes, now you're stuck again, either propagating the localstack changes into your other addon, or you're in an either/or situation... I think I'm good merging this as-is, and waiting for a composable solution. |
nimakaviani
left a comment
There was a problem hiding this comment.
sounds good.
Frankly, I am not too concerned about the fact that we will do a full override fore cm-coredns.yaml. We can automate that bit via some scripting to ensure what we get in the localstack folder extends what's in there for the ref implementation. The part that was more annoying was the bit on rewriting the git tree.
But yeah, lets leave that as an open issue and we will explore alternatives.
This adds localstack as an addon to the ref implemention, which involves the following:
I already attempted to add the same directly to the ref impl here, but there were lots of cross-cutting concerns there. This is more of a drop-in, although doesn't integrate quite as much.
I think this should be much more acceptable, despite the minor UX downsides.