IaC: Add AWS operations and CFN resources catalog #13027
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 8m 18s ⏱️ Results for commit 961dbee. ♻️ This comment has been updated with latest results. |
bentsku
left a comment
There was a problem hiding this comment.
This is nice 👌 super nice to see how easy it is to plug into this! this is only a pre-review as I got the notification, I'm still unsure how we are going to extend the behavior from Pro. Might be good to try it already to see how we could do it, and adapt the code in consequence? You might be on it already 😄
| def map_catalog_availability_to_exception( | ||
| service_name: str, | ||
| operation_name: str | None, | ||
| support_status: AwsServicesSupportInLatest | AwsServiceOperationsSupportInLatest, | ||
| ) -> AwsServiceAvailabilityException: |
There was a problem hiding this comment.
question: how will we extend this with more possible values in the Pro version? we might want to aim for it to be extended transparently, as we won't be able to plug into the Skeleton.on_not_implemented_error function directly to modify which function is being called.
We could probably patch it, and execute some code before deferring the logic to this function? maybe there are better ways. Patching might do, we're also using it for our AWS routing.
Thanks, yes, I will extend this in pro tomorrow :) I also want to see how the whole integration will work before merging this PR |
bentsku
left a comment
There was a problem hiding this comment.
very nice addition with the plugins 🚀 thanks for addressing the comments, and making this looking really great and extendable!
I believe the RemoteCatalogLoader is currently broken and will break the functionality all together, but once this is addressed, I think we would be good to go 👍
Only have a few comments, agreeing with Silvio, and added some more related to some Enum being returned. Nice work 👌
b2b69dc to
92e58e9
Compare
|
I removed integration to service and skeleton to merge this PR faster |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 37m 39s ⏱️ Results for commit 0688e84. ♻️ This comment has been updated with latest results. |
bentsku
left a comment
There was a problem hiding this comment.
LGTM, I only have some questions around the fallback for the Pro "runtime-only" Catalog, but this could also be addressed in a follow-up PR 👍, once we know more.
Thanks for splitting the PR so that we don't alter the behavior right away! 🚀
| except Exception as e: | ||
| LOG.debug( | ||
| "Failed to load catalog plugin with the latest LocalStack services support data, falling back to catalog without remote state: %s", | ||
| e, | ||
| ) | ||
| return plugin_manager.load("aws-catalog-runtime-only") |
There was a problem hiding this comment.
something I missed: I believe we should maybe raise a specific exception we would have subclassed ourselves in case we have a failure in the RemoteCatalogLoader, so that we could probably guard only against this? because here, if we have any kind of exceptions when instantiating the Catalog, we would fallback, even if the exception is something we should fix, like a Key error or something?
There was a problem hiding this comment.
Notes from discussion: agreed that for now it is ok to fall back to the default runtime catalog in case any exception is thrown during catalog initialization.
Motivation
This PR introduces a plugin AWS license catalog which will be used to customize error messages for unimplemented AWS services or operations.
Downloading and loading of the license catalog is not implemented in this PR and will be completed in the next PR.
Changes