feat!: convert to copier-managed skeleton - #11
Conversation
…donly test wiring The primitive module's own copier conversion (v2.0.0) is a pure major-version bump with no interface changes, but the pessimistic ~> 1.0 constraint here never resolves to it. Also corrects tests/post_deploy_functional_readonly, which called lib.RunSetupTestTeardown (apply/destroy) instead of lib.RunNonDestructiveTest, and imported testimpl from the primitive module instead of this repo's own tests/testimpl package.
bryce-lynn-nttd
left a comment
There was a problem hiding this comment.
APPROVE — First reference-module conversion; clean mechanical skeleton migration plus two well-justified follow-ups. Verified against head ba17fa6.
- Scope is tight and correct. Conversion touches only tooling/CI/agent-config; the Terraform interface is untouched except the single load-bearing line in
main.tf..lcafenvremoved,.copier-answers.ymlpinned to skeleton0.7.2, workflow SHAs single-sourced tolaunch-workflows@0.15.4. - Primitive constraint bump is safe.
~> 1.0→~> 2.0onmodule.lambda_functioncorrectly consumes the now-converted primitive. Confirmedlambda_function2.0.0is a real release with an empty interface diff vs1.0.7, so the bump is API-compatible — consistent with the reported 15-add / 0-change / 0-destroy plan.fix!framing is the conservative-correct call. - Readonly test wiring corrected. Entrypoint moved from
RunSetupTestTeardowntoRunNonDestructiveTest, and the import fixed from the primitive's package to this repo's owntests/testimpl.TestComposableCompleteis genuinely non-mutating (onlyGetFunctionreads with specific-valueassert.Equalon ARN / name / override-name), so it's valid for both the readonly and functional suites. The newvalidate-readonly-testhook correctly caught the pre-existing defect. - CI fully green.
Nice work — the reference-tier flow holds up.
Generated with Cursor Agent (Opus 4.8)
rakesh-gorige-nttd
left a comment
There was a problem hiding this comment.
PR Review: feat!: convert to copier-managed skeleton (#11)
Repo: launchbynttdata/tf-aws-module_reference-bulk_lambda_function
Author: @oscar-hernandez-nttd
Branch: feat!/copier-conversion → main
Jira: EN-3436
Head: ba17fa6 (2 commits)
What This PR Does Well
- Clean two-commit structure. Separating the mechanical skeleton conversion (
feat!:) from the load-bearing follow-ups (fix!:) makes review and release notes much easier to follow. The PR description explains the rationale well. - First reference-architecture conversion. Good pilot for the batch — proves the copier flow works one level above primitives without touching the module's Terraform interface (except the version constraint).
- Primitive constraint bump is correct and necessary.
~> 1.0→~> 2.0onmodule.lambda_functionis required to consume the converted primitive. I verifiedlambda_function2.0.0has no diff onmain.tf,variables.tf, oroutputs.tfvs1.0.7— the bump is semver-only, not an API change. - Readonly test wiring is properly fixed. The pre-existing defects were worse than just the wrong runner:
RunSetupTestTeardown→RunNonDestructiveTest(was doing full apply/destroy in a "readonly" suite)- Import fixed from the primitive's
tests/testimplto this repo's owntests/testimpl— the old import would have exercised the wrong module config and wrong assertions (single-function vs bulk map outputs) - Entrypoint renamed to
TestBulkLambdaFunctionsModuleReadOnlyfor clarity
TestComposableCompleteis genuinely non-destructive. It only callsGetFunctionand uses specific-valueassert.Equalon ARNs/names/overrides — valid for both functional and readonly suites.- Skeleton enrollment looks correct.
.copier-answers.ymlpinned to skeleton0.7.2; workflow SHAs consistently point tolaunch-workflows@edddecd(tag0.15.4); legacy workflows removed as expected. - CI fully green — pre-commit, Terraform lint, Terraform tests (including apply/assert/destroy), and legacy status checks all pass.
Fixes Required
Blocking
No blocking issues found.
Non-Blocking
| # | File & Line | Issue |
|---|---|---|
| 1 | .github/release-drafter.yml |
autolabeler section removed (branch-name → major/minor/patch labeling). This is intentional per skeleton standardization — labeling now comes from the conventional-commit PR title workflow (breaking change, feature, etc.). No action needed unless the team wants to document this for reference-module consumers. |
| 2 | PR title | Title is feat!: convert to copier-managed skeleton but the second commit is fix!: . CI accepted it, and the PR body covers both commits well. Consider noting in merge commit / release notes that this PR ships both a skeleton conversion and a primitive constraint bump. |
| 3 | tests/post_deploy_functional_readonly/main_test.go |
RunNonDestructiveTest requires pre-existing deployed state. That's correct by design, but worth confirming in team docs that the readonly suite is for post-deploy validation only (not CI apply/destroy). |
Summary
This is a strong reference-tier copier conversion. The skeleton migration is mechanical and complete, and the follow-up commit addresses two real pre-existing problems that the new validate-readonly-test hook correctly surfaced. The primitive version bump is safe and necessary.
Recommendation: APPROVE
Test Plan Verification
| Item | Status |
|---|---|
Pre-commit / validate-readonly-test |
CI pass |
| Terraform lint + validate + plan | CI pass |
| Functional suite (apply → assert → destroy) | CI pass |
| Readonly suite statically non-destructive | Verified in diff |
| No module interface change beyond version constraint | Verified |
Summary
Converts this repository to the copier-managed skeleton layout, bringing its CI/CD tooling, linting configuration, and agent guidance in line with the rest of the Launch Terraform module library. This is the first reference-architecture / collection module converted under this effort (prior conversions were primitives only) — a test run of the same flow one level up the stack.
Jira: EN-3436
This PR contains two commits:
feat!: convert to copier-managed skeleton— the automated conversion viaupdater.sh, delivering updatedMakefile, GitHub workflows,.agents/reference docs,.tool-versions, and related tooling/config files. This is a breaking change for anyone relying on the previous hand-maintained CI setup. It intentionally does not touch anything undertests/or the module's Terraform interface.fix!: bump lambda_function primitive constraint to ~> 2.0 and fix readonly test wiring— two required follow-ups surfaced by consuming the now-converted primitive and by the skeleton's own new tooling:main.tfpinned thelambda_functionprimitive withversion = "~> 1.0". That primitive was itself converted and released as2.0.0in #48 — a purefeat!version bump with an emptyvariables.tf/outputs.tf/main.tfdiff against1.0.7. A pessimistic~>constraint on1.0never resolves to2.0.0, so without this bump the conversion here would silently keep consuming the old, unconverted primitive..github/scripts/validate-readonly-test.sh, a new pre-commit hook that enforces a testing convention this repo'stests/directory was already violating before this conversion — nobody was checking for it until now.tests/post_deploy_functional_readonly/main_test.gocalledlib.RunSetupTestTeardown(a full apply/destroy cycle, not read-only) and importedtestimplfrom the primitive module's package instead of this repo's owntests/testimpl. This commit rewires the entrypoint tolib.RunNonDestructiveTestand fixes the import to use this repo's owntestimpl.TestComposableComplete, which is already non-mutating (read-onlyGetFunctionassertions).It's a
fix!:rather than folded into thefeat!:commit because it corrects pre-existing defects the conversion's own new tooling and the primitive's release surfaced, not the conversion itself. It's marked breaking (!) because of the version constraint bump.Test plan
make checkpasses locally (AWS_PROFILE=020127659860_AdministratorAccess AWS_REGION=us-east-2), including a real apply → assert → destroy cycle for the functional suite; plan shows 15 to add / 0 to change / 0 to destroy, confirming the version bump causes no resource replacement.validate-readonly-testhook passes.tests/post_deploy_functional_readonlybuilds and is statically confirmed non-destructive (RunNonDestructiveTestrequires pre-existing deployed state and never calls apply/destroy).terraform.tfstateunderexamples/*/after the run.