-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: address PR1 review comments — EKS-only Lambda + Terraform cleanup
- Rewrite app.py to ~380 lines (EKS + CodeBuild only); remove all generic
GitHub/GitLab/template-manager code paths
- Delete 7 dead modules: repository_provider, github_provider, gitlab_provider,
github_client, gitlab_client, template_manager, models
- Delete eks_config.py (Jinja2 rendering now handled entirely by terraform-eks-
deployment in CodeBuild)
- Rewrite tests/test_app.py for EKS-only handler; drop stale test files
- Simplify requirements.txt (remove requests/requests-mock; now using urllib)
deploy/ Terraform:
- Add conditional CodeBuild IAM role (create_codebuild_role var); default=true
creates a minimal role with scoped logs/S3/VPC policies; false looks up a
pre-existing role by name
- Add data sources for subnet + security group lookup by name (no hardcoded IDs)
- Replace hardcoded 'ghe-runner/github-token' with var.tf_github_token_secret_name
- Fix IAM policy partition: arn:aws-us-gov: -> arn:${partition}:
- Remove aws_vpc_endpoint.codebuild (VPC endpoints already exist in the account)
- Remove codebuild_role_arn / codebuild_vpc_id / subnet_ids / security_group_ids
variables; add codebuild_role_name, subnet_name, security_group_name,
tf_github_token_secret_name, codebuild_assets_bucket
service-catalog/product-template.yaml:
- Fix ServiceToken partition: arn:aws-us-gov:lambda: -> arn:${AWS::Partition}:lambda:
csvd_config_packer.hcl:
- Annotate account-specific fields with (*) comments explaining they cannot be
dynamically resolved by packer-pipeline HCL- Loading branch information
Your Name
committed
Apr 14, 2026
1 parent
065d2f2
commit 560a5ec
Showing
23 changed files
with
851 additions
and
5,147 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| # PR #1 Review — Implementation Plan | ||
|
|
||
| Addresses all comments from morga471 on | ||
| https://github.e.it.census.gov/CSVD/lambda-template-repo-generator/pull/1 | ||
|
|
||
| --- | ||
|
|
||
| ## A. Remove the generic code path | ||
|
|
||
| The Lambda was built on top of an older generic repo-creation-from-Python framework | ||
| (GitHub/GitLab providers, config.json rendering, template manager). Now that all repo | ||
| creation runs through CodeBuild + terraform-eks-deployment, none of this code is | ||
| reachable in production. Remove it entirely. | ||
|
|
||
| | # | File | Change | | ||
| |---|------|--------| | ||
| | A1 | `template_automation/app.py` | Gut to ~150 lines: keep only CFN event parsing (`handler`), `start_codebuild_build()`, `poll_codebuild_build()`, post-build PR URL fetch, and `cfn_response()`. Remove all `GitHubProvider`/`GitLabProvider` instantiation, `RepositorySettings`, `MergeRequestSettings`, `FileContent`, `config.json` write, the generic `if not is_eks_deployment` branch, and `to_template_settings()` | | ||
| | A2 | `template_automation/app.py` | Remove `CloudFormationResourceInput.to_template_settings()` method; simplify model to only CFN parsing + `is_eks_deployment` check + `to_eks_deployment_config()` | | ||
| | A3 | Delete | `template_automation/repository_provider.py` | | ||
| | A4 | Delete | `template_automation/github_provider.py` | | ||
| | A5 | Delete | `template_automation/gitlab_provider.py` | | ||
| | A6 | Delete | `template_automation/github_client.py` | | ||
| | A7 | Delete | `template_automation/gitlab_client.py` | | ||
| | A8 | Delete | `template_automation/template_manager.py` | | ||
| | A9 | Delete | `template_automation/models.py` | | ||
| | A10 | `template_automation/requirements.txt` | Remove deps no longer needed (e.g. `pygithub`, `python-gitlab`) | | ||
| | A11 | `template_automation/templates/` | Remove `config.json` template if it only served the generic path; remove directory if empty | | ||
|
|
||
| --- | ||
|
|
||
| ## B. Terraform infra fixes (from Matt's inline comments) | ||
|
|
||
| | # | File | Matt's comment | Change | | ||
| |---|------|----------------|--------| | ||
| | B1 | `deploy/main.tf` | "Wouldn't we always want to create the role?" | Add `resource "aws_iam_role"` + `aws_iam_role_policy_attachment` to create the CodeBuild execution role in this module; remove dependency on pre-existing role | | ||
| | B2 | `deploy/main.tf` | "pass in token secret name" / "Should also pass in the secret name" | Replace hardcoded `"ghe-runner/github-token"` string in Lambda env vars and IAM policy ARN with `var.tf_github_token_secret_name` | | ||
| | B3 | `deploy/main.tf` | "look up the partition value with data.aws_caller_identity.current.partition" | Replace `"arn:aws-us-gov:secretsmanager:..."` with `"arn:${data.aws_caller_identity.current.partition}:secretsmanager:..."` (caller_identity already declared) | | ||
| | B4 | `deploy/main.tf` | "We shouldn't create VPC endpoints, they should already be in the account we use." | Remove `aws_vpc_endpoint.codebuild` resource entirely | | ||
| | B5 | `deploy/main.tf` | — | Add `data "aws_subnet"` + `data "aws_security_group"` lookups by name/tag to replace hardcoded IDs passed as variables | | ||
| | B6 | `deploy/variables.tf` | "This should be looked up so it can work across accounts." | Remove `codebuild_role_arn` variable (role now created in module per B1); add `tf_github_token_secret_name` variable (default `"ghe-runner/github-token"`) | | ||
| | B7 | `deploy/variables.tf` | — | Remove `codebuild_vpc_id` variable; add subnet/SG name filter variables to drive data sources (B5) | | ||
| | B8 | `deploy/terraform.tfvars` | "These should be looked up or created" | Replace hardcoded `subnet_ids`, `security_group_ids`, `codebuild_vpc_id` IDs with name-based values that feed data source lookups | | ||
| | B9 | `csvd_config_packer.hcl` | "This should be looked up by Name, partition, account id" / "These should be looked up or created" | Replace hardcoded `account_number`, `partition`, `codebuild_role_arn`, `vpc_id`, subnet/SG IDs — drive from env vars resolved at build time via `aws sts get-caller-identity` / `aws iam get-role` wrapper | | ||
|
|
||
| --- | ||
|
|
||
| ## C. CFN template fix | ||
|
|
||
| | # | File | Matt's comment | Change | | ||
| |---|------|----------------|--------| | ||
| | C1 | `service-catalog/product-template.yaml` **and** `terraform-service-catalog-census/templates/products/eks-terragrunt-repo/2-0-0.yaml` | "look up partition" | Change `arn:aws-us-gov:lambda:` → `arn:${AWS::Partition}:lambda:` in the `ServiceToken` | | ||
|
|
||
| --- | ||
|
|
||
| ## D. PR response comment | ||
|
|
||
| | # | Action | | ||
| |---|--------| | ||
| | D1 | Reply to Matt's comment on `repository_provider.py`: "Good call — we're removing the entire generic code path (A1–A11 above). The file won't be needed." | | ||
|
|
||
| --- | ||
|
|
||
| ## Notes | ||
|
|
||
| - A and B are independent; either can be done first. | ||
| - C1 must be applied to **both** copies of the template and kept in sync. | ||
| - After B1 (create CodeBuild role in Terraform), run `tf apply` in `deploy/` and update | ||
| `deploy/terraform.tfstate` before rebuilding the Lambda image. | ||
| - After all changes, rebuild the Lambda image (packer CodeBuild build) and force a Lambda | ||
| update (`aws lambda update-function-code --image-uri ...`) before running the e2e test. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.