refactor(cloudformation): move the single-service types to provisioners - #2744
refactor(cloudformation): move the single-service types to provisioners#2744hectorvent wants to merge 1 commit into
Conversation
|
| Filename | Overview |
|---|---|
| src/main/java/io/github/hectorvent/floci/services/cloudformation/CloudFormationResourceProvisioner.java | Removes the migrated resource-type branches and delegates their ownership to the provisioner registry. |
| src/main/java/io/github/hectorvent/floci/services/cloudformation/provisioners/EcrCfnProvisioner.java | Extracts ECR repository provisioning and deletion, while the previously reported update-time recreation behavior remains. |
| src/main/java/io/github/hectorvent/floci/services/cloudformation/provisioners/KmsCfnProvisioner.java | Extracts KMS key and alias lifecycle handling, including unconditional key creation when provisioning is invoked for an update. |
| src/main/java/io/github/hectorvent/floci/services/cloudformation/provisioners/SsmCfnProvisioner.java | Extracts SSM parameter provisioning and deletion but continues to run put/create-style logic on updates. |
| src/main/java/io/github/hectorvent/floci/services/cloudformation/provisioners/FirehoseCfnProvisioner.java | Extracts Firehose stream provisioning and deletion while retaining create-style behavior for update invocations. |
| src/main/java/io/github/hectorvent/floci/services/cloudformation/provisioners/PipesCfnProvisioner.java | Extracts EventBridge Pipes provisioning and deletion while retaining unconditional creation on updates. |
| src/main/java/io/github/hectorvent/floci/services/cloudformation/provisioners/CdkMetadataCfnProvisioner.java | Replaces stub ownership with a real provisioner but still generates a new metadata identity whenever provisioning runs. |
| src/test/java/io/github/hectorvent/floci/services/cloudformation/provisioners/LeafCfnProvisionerTest.java | Adds broad create/delete delegation and resource-contract coverage for the extracted provisioners. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
CFN[CloudFormation stack operation] --> Registry[Resource provisioner registry]
Registry --> KMS[KmsCfnProvisioner]
Registry --> SSM[SsmCfnProvisioner]
Registry --> ECR[EcrCfnProvisioner]
Registry --> Pipes[PipesCfnProvisioner]
Registry --> Firehose[FirehoseCfnProvisioner]
Registry --> CDK[CdkMetadataCfnProvisioner]
KMS --> KmsService
SSM --> SsmService
ECR --> EcrService
Pipes --> PipesService
Firehose --> FirehoseService
CDK --> Identity[Metadata physical ID]
Reviews (2): Last reviewed commit: "refactor(cloudformation): move the singl..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Map<String, String> tags = parseCfnTags(props != null ? props.get("Tags") : null, ctx); | ||
|
|
||
| Repository repo; | ||
| try { |
There was a problem hiding this comment.
When UpdateStack dispatches with the prior physical ID and attributes, these extracted provisioners ignore that state and execute create logic again, causing migrated resources to be recreated or overwritten and CDK metadata to receive a new identity.
Context Used: AGENTS.md (source)
pgermosen
left a comment
pgermosen
left a comment
There was a problem hiding this comment.
Approving. On greptile's finding about the update path recreating resources / CDK metadata getting a new identity each time: I pulled the exact pre-migration provisionEcrRepository and provisionCdkMetadata methods from the monolith's history and diffed them against these new provisioners. Both are byte-identical in behavior — no create-vs-update branching existed before this move either. So this is a real, pre-existing gap across the CFN provisioner set, not something this migration introduced. The "ported verbatim" claim holds up.
One small correction for the description: CDK::Metadata actually had its own dedicated provisionCdkMetadata case before, not the generic stub arm — doesn't change anything functionally, both generated a random UUID every call.
Migrates the seven resource types whose provisioning is one service call: KMS Key and Alias, SSM Parameter, ECR Repository, Pipes Pipe, Kinesis Firehose DeliveryStream, and CDK Metadata. Behaviour-preserving move: the bodies are ported verbatim apart from the mechanical rewrites onto ProvisionContext, so physical ids, Fn::GetAtt keys and delete calls are unchanged. Each provisioner injects only the service it wraps, so five service fields leave the monolith. Their constructor parameters stay, unused, until the final cleanup drops them all at once; shrinking the signature now would churn the fixture for no gain. parseCfnTags, blankToNull and parseIntProp are copied rather than shared, because each still has callers in the monolith. ProvisionContext.resolveTags is deliberately NOT substituted for parseCfnTags: it skips a blank key that parseCfnTags keeps, orders entries by insertion rather than hash, and resolves the whole property so an Fn::If around the list works. Adopting it is a behaviour change and belongs in its own PR. CDK::Metadata gains a real provisioner rather than staying on the stub arm. The stub reports CREATE_COMPLETE with a synthetic id and a fake Arn attribute; the real type has no attributes at all. Tests assert the exact physical id and the exact Fn::GetAtt keys against each type's registry schema, plus delegated arguments and delete calls. Asserting status alone would pass against the stub arm.
6338b33 to
4482420
Compare
4389ca2 to
21dae0f
Compare
2 participants
Summary
First slice that actually moves types. Migrates the seven whose provisioning is a single service
call, out of
CloudFormationResourceProvisioner's switch and into per-service provisioners:KmsCfnProvisionerAWS::KMS::Key,AWS::KMS::AliasSsmCfnProvisionerAWS::SSM::ParameterEcrCfnProvisionerAWS::ECR::RepositoryPipesCfnProvisionerAWS::Pipes::PipeFirehoseCfnProvisionerAWS::KinesisFirehose::DeliveryStreamCdkMetadataCfnProvisionerAWS::CDK::MetadataBehaviour-preserving. Bodies are ported verbatim apart from the mechanical rewrites onto
ProvisionContext(engine→ctx.engine(), region/account/stack →ctx.*,resolveOptional,generatePhysicalName), so physical ids,Fn::GetAttkeys and delete calls are unchanged. Thegenerated docs table is byte-identical after this change: only the owner moved, not the types.
Monolith: 7,603 → 7,396 lines; legacy switch 79 → 72 types.
Stacked on #2739 → #2732. Review those first.
Two judgement calls worth flagging
ProvisionContext.resolveTagswas deliberately not substituted forparseCfnTags. They arenot equivalent:
resolveTagsskips a blank key thatparseCfnTagskeeps, orders entries byinsertion rather than hash, and resolves the whole property so an
Fn::Ifaround the list works.Swapping would be a silent behaviour change in a move PR, so
parseCfnTagsis copied into thethree provisioners that need it (as are
blankToNullandparseIntProp), each still havingcallers left in the monolith. Adoption is filed as a follow-up.
Five service fields left the monolith, but their constructor parameters stayed. Unused
parameters are legal, and shrinking the signature now would churn
CfnProvisionerFixturefor nogain; the final cleanup drops all of them at once.
AWS::CDK::Metadatagains a real provisioner rather than being left to the stub arm. The stubreports
CREATE_COMPLETEwith a synthetic id and a fakeArnattribute; the real type has noattributes at all, so the stub was quietly wrong.
Type of change
fix:)feat:)feat!:orfix!:)refactor(cloudformation):— no user-visible behaviour change, release-neutral.AWS Compatibility
N/A for the wire protocol. Every migrated type keeps its exact physical id and
Fn::GetAttattribute keys, verified per type against its CloudFormation registry schema:
primaryIdentifier/readOnlyPropertiesKMS::KeyKeyId/Arn,KeyIdKMS::AliasAliasName/ noneSSM::ParameterName/ noneName/Type/ValueECR::RepositoryRepositoryName/Arn,RepositoryUriPipes::PipeName/Arn, +4Arn(see follow-up)KinesisFirehose::DeliveryStreamDeliveryStreamName/ArnChecklist
./mvnw testpasses locallyCloudFormation + Cloud Control: 835 tests, 0 failures, 0 errors.
LeafCfnProvisionerTestadds20 covering every migrated type: exact physical id, exact
Fn::GetAttkeys, delegated argumentsvia captors, and delete delegation. The pre-existing CloudFormation integration tests were not
touched, which is the real evidence nothing regressed.
The inventory guard from #2732 is what proves the migration actually took effect: it compares the
checked-in inventory against the CDI-resolved registry, so a provisioner missing
@ApplicationScoped(silently never registered, its types quietly stubbed) fails the build.Follow-ups, deliberately not fixed here
AWS::Pipes::Pipesets onlyArn. The registry schema also listsCreationTime,CurrentState,LastModifiedTimeandStateReasonas read-only, soFn::GetAtton any ofthose resolves to the literal
"LogicalId.Attr". Pre-existing, carried over unchanged.ProvisionContext.resolveTagsacross theparseCfnTagscall sites once thebehaviour difference is settled, and delete the copies.