[Feature] Service-specific ACKAddOns #978
Conversation
- Added a service-specific ACKAddOn class per all AWS services in serviceMappings to allow ARM support of ACK services that support ARM - Added unit tests for service-specific ACKAddOns S3 (supports all platforms) & MemoryDB (only serviceMappings service which does not support ARM) - Added service-specific ACKAddOn classes as exports of ack/index.ts
…creation more robust - Kinesis & KMS annotations were missed & are now added - Improved tests so that it covers all service-specific addons - Added test to confirm it will error if an addon with an unsupported architect and supported architecture are added to the same build - Modified createNamespace logic to check if the addon namespace exists before attempting to create it
logic was added as test for adding multiple ACKAddOns was failing as both addons were trying to create the same namespace. It was most likely an intentional design choice for the user to override the default prop to create namespace, as the prop comment mentions it. Modified test so that only the first ACKAddOn added to the list spefies to create namespace and others do not
dwright20
requested review from
aws-ia-ci,
parkand1,
shapirov103,
youngjeong46,
elamaran11 and
a team
as code owners
There was a problem hiding this comment.
@dwright20 First of all thankyou so much for you PR. Especially liked the test scripts a lot. Fundmentally, I dont think we should use this design approach of one class per ACK chart, this will make the design difficult the expand for future addons. I think we should stick to existing design with following as design choices:
- Add
supportsALLto original ACK addon and create another enum of list of services which do not support ARM, In theACKAddonif the AWS Service is in the ENUM list of non-ARM supported like memoryDB and if Nodes are ARM, we should error out. - Add
supportsALLto original ACK addon along with adding a documentation to say ARM is not supported today for MemoryDB (This is more of temp solution).
@shapirov103 Any other design choices?
| extends Omit<AckAddOnProps, "serviceName"> { } | ||
|
|
||
| @supportsALL | ||
| export class ACMAckAddOn extends AckAddOn { |
There was a problem hiding this comment.
I dont think we should use this design approach of one class extension per ACK chart, this will make the design difficult the expand.
There was a problem hiding this comment.
@dwright20 seems like a bit of duplication going on, but on the other hand it creates a placeholder for each ACK.
Ideally, we would like to keep the approach generic so that little maintenance is needed.
I don't want to dismiss the design choice, however, let's use the principle of "as simple as possible but not simpler".
Let's say we change the existing approach slightly and add the target CPU architecture to the serviceMappings.ts, then implement this support in the AckAddOn. We will also add supportsAll to the AckAddOn by default and add code that will handle exclusions. This can be done by replicating some of the code that we have in the supportsX86 and such decorators.
If the above is implemented, what else is missing that requires explicit class per Ack controller implementation? Do you envision additional use cases?
|
@dwright20 please see my comment and let me know what you think. I mentioned an approach to simplify the design approach to make it easier for us to maintain. This has been open for a while. |
|
This PR has been automatically marked as stale because it has been open 60 days |
Issue #, if available: 974
Description of changes:
@supportsALLor@supportsX86per ACK RegistryEksBlueprint.builder()) & an ARM builder (GravitonBuilder())By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.