Repository navigation
feat(aws-startup-advisor): Heroku-to-AWS day-to-day command cheat sheet in the heroku-to-aws migration guide - #615
Conversation
…at sheet to the heroku-to-aws migration guide The guide covered provisioning, data, cutover and rollback and then stopped; nothing told developers what `heroku logs --tail`, `run bash`, `config:set`, `ps:scale`, `releases`, `rollback`, `maintenance:on` or `pg:psql` become. - MIGRATION_GUIDE.md: new "Day-to-day commands: Heroku -> AWS" section (TOC entry; one table per compute target present, rows filled with the app's generated names so they paste). Closing paragraph names the two commands with no one-liner (push-to-deploy -> Phase 3 workflow; pipelines:promote -> not generated) and suggests bin/ wrappers as a first platform-team task. - ecs.tf: enable_execute_command = true on each service plus the ssmmessages actions on the task role, so the `heroku run bash` row (ECS Exec) is true. - Template Variable Resolution: EB_WEB_ENVIRONMENT and ECS_CLUSTER.
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖] Reviewed 4fe0e588f8ad4382d21b4b7d56fdbc394ab29bc1. The new ECS Exec policy fails the mandatory generation policy gate, and the command reference disappears when deferred is empty. When the tables render, configuration backends, EB logging, mixed/worker-only targets, HTTPS hostnames, Aurora snapshots and Alpine shells need the corrections listed inline. These eight items are the consolidated correction list for this review.
Local validation: 83 migration-artifact tests and 121 subtests passed; 60 policy tests passed; Markdown lint, vendored parity and git diff --check passed. The extracted IAM template passes the policy checker at the base and fails at this head. Template-condition projections confirm the empty-deferred omission. No live AWS deployment was performed.
…the design and gate ECS Exec honestly Review findings on PR aws#615. - validate-terraform-policy.py: no_wildcard_iam now exempts a second statement shape, an Allow whose Action list holds only the four ssmmessages channel actions (ECS Exec), because AWS defines no resource-level permissions for them. The exemption is statement-shaped: any other action in the same list (ssmmessages:*, ssm:*, s3:*) or a second wildcard statement in the same policy is still rejected. Rule docs (security-posture-rules.md, SKILL.md) updated; tests cover the emitted generate-terraform.md statement passing and the mixed cases failing. - generate-docs.md: the "Day-to-day commands" section now sits before the deferred_addons guard so it renders with an empty deferred list. Rows render per process type from eb_processes[] / fargate_processes[] (no hardcoded -web service, task family or container); maintenance and open rows are gated on app_has_eb_web / app_has_fargate_web; app-wide actions loop over every environment/service. Config rows follow the actual backing store (plain property, Secrets Manager ARN, SSM parameter) with explicit identifiers taken from beanstalk.tf/ecs.tf instead of an invented /<app>/KEY path or <app>/DATABASE_URL secret. EB log tail uses the full .../eb-current-app/stdouterr.log group; shells use /bin/sh (Alpine images); pg rows are gated on has_postgres and branch RDS create-db-snapshot vs Aurora create-db-cluster-snapshot; open uses https://\<custom domain> when configured, else http://\<EB CNAME> (the default environment has no HTTPS listener). - generate-terraform.md: beanstalk.tf enables aws:elasticbeanstalk:cloudwatch:logs StreamLogs=true with RetentionInDays so the tailed group exists. - tests/test_heroku_command_cheatsheet.py pins all of the above. Verification: `mise run ci` passed (lint:md, lint:artifacts, lint:vendored-parity, lint:skill-refs, pytest 1483 passed, telemetry 311 passed); skills/tf-best-practices/scripts/test_validate_terraform_policy.py 63 passed; the extracted ecs_task_exec block returns POLICY_OK.
herosjourney
left a comment
There was a problem hiding this comment.
Reviewed: head 4fe0e58 (base 00840a0), 2 files (+82/−0). Only heroku-to-aws is touched, in the single plugins/aws-startup-advisor tree.
Ran:
pytest plugins/aws-startup-advisor/skills/heroku-to-aws: 112 passed.pytest skills/tf-best-practices tests: 1535 passed, 225 subtests.- Policy-gate probe:
validate-terraform-policy.pyon the PR'secs_task+ecs_task_execHCL givesPOLICY_FAIL rule=no_wildcard_iam. The same file withoutecs_task_execgivesPOLICY_OK. aws <svc> <cmd> help(CLI 2.31.1) for all 20 subcommands the cheat sheet uses, plus their required flags.- An offline shorthand/JSON parse check (dummy credentials, dead endpoint) for
elbv2 modify-listener, EB--option-settings,ecs run-taskand the--queryexpressions: all parse. {{IF}}/{{ENDIF}}balance: 92/92.- The TOC slug matches the heading.
GitHub CI is green at head (startup-advisor, CodeQL, author checklist); it is not restated here.
Primary sources (read 2026-11-10):
- EB "Fetching secrets and parameters to environment variables"
- EB "Using Elastic Beanstalk with CloudWatch Logs"
- EB command-options-general (
StreamLogsdefaults tofalse,SystemTypetobasic) - managed policies
AWSElasticBeanstalkWebTierandAmazonECSTaskExecutionRolePolicy - Service Authorization Reference for
ssmmessages describe-environment-healthCLI help
Did not run: live AWS calls, an end-to-end Generate run, or terraform validate. The EB CLI and Session Manager plugin are not installed locally.
Cross-plugin parity: n/a. Single tree.
Merge state: clean into main. It shares both files with #614 with no textual conflict.
Change narrative:
- Problem: the generated guide stops at decommission, so Heroku developers get no "what do I type now".
- Mechanism: two cheat-sheet tables (EB and Fargate) with the generated names filled in, plus ECS Exec enabled in the Fargate Terraform.
- Body vs diff:
- The body says "the generated
ecs.tf", but the generator emitssecurity.tfandcompute.tf. - The body says "Independent of the other open heroku-to-aws PRs", but
generate-docs.md:1423points at a section only #612 adds.
| Axis | Stance | One line |
|---|---|---|
| Parsimony | advisory | Six of the 16 rows are byte-identical across the two tables. Acceptable for paste-ability. |
| Completeness | finding | The new IAM statement fails the hard policy gate. Several EB rows depend on settings beanstalk.tf never emits (log streaming, SSM access, enhanced health). |
| Security | advisory | The four ssmmessages actions on * are the AWS-documented minimum; three of the four support no resource type. ECS Exec is on for every service, with no opt-out. |
| Alternatives | advisory | Two below. |
| Contradictions | finding | "Nothing here needs to be built" is not yet true. CLI scale and rollback revert on the next terraform apply and bypass writers_quiesced. |
| Overlap | clear | The config rows restate the Config Var Migration sections; see Propagation. |
| Cross-plugin parity | clear | Single tree. |
| Propagation & data | finding | The ECS cluster, service, family, log group and container names match the Terraform. The EB application name, the SSM path and the DB secret id do not. |
Blocking
- [Completeness — gate vs emitter] The new ECS Exec policy fails
no_wildcard_iam, so every Fargate Generate run ends inGATE_FAIL—references/phases/generate/generate-terraform.md:1074
Resource = "*"(inaws_iam_role_policy.ecs_task_exec)
- The gate:
generate-assemble.md:44-59runsvalidate-terraform-policy.pyas a hard gate and assertspolicy_status == "POLICY_OK"(:103-104). The validator's onlyResource "*"exemption is an isolatedelasticbeanstalk:CreateStorageLocationstatement (validate-terraform-policy.py:763-780). - Reproduced: base IAM gives
POLICY_OK; base IAM plus this resource givesPOLICY_FAIL | rule=no_wildcard_iam. - Why the retry loop can't recover: the
fix_hintsays "scope the policy". ScopingCreateControlChannel,CreateDataChannelorOpenControlChannelbreaks ECS Exec, because they support no resource type. Deleting the policy makes theheroku run bashrow false. - Why CI is green: no tf-best-practices fixture renders the Fargate IAM block.
Fix:
- Keep the HCL.
- Add a second statement-shaped exemption in
_remove_allowed_resource_wildcard_statementsthat matches exactly the fourssmmessagesactions withResource "*". On a scratch copy this gavePOLICY_OKfor the isolated statement,POLICY_FAILwhens3:GetObjectwas added to it, and kept the existing codepipeline fixture atPOLICY_OK. - Do not switch to
data "aws_iam_policy_document": the validator fails open on data sources, so that would hide the statement instead of satisfying the gate. - Regression: a
good-heroku-fargate-ecs-execfixture, plus a mixed-statement negative test.
Should-fix
-
The EB
run bashand one-off rows fail on the generated environment —generate-docs.md:1381-1382.beanstalk.tfsets noEC2KeyNameand no :22 ingress, soeb sshcannot connect. The instance role has onlyAWSElasticBeanstalkWebTierplus read-secrets, with nossm:*/ssmmessages:*, sostart-sessionalso fails. Fix: attachAmazonSSMManagedInstanceCoretoaws_iam_role.eb_instance_<app>, dropeb ssh, and resolve the instance ID withaws ec2 describe-instances --filters Name=tag:elasticbeanstalk:environment-name,…. -
The EB logs row tails a log group that does not exist —
generate-docs.md:1380.StreamLogsdefaults tofalseand is never set, and the group name ends ineb-current-app/stdouterr.log. Fix: emitaws:elasticbeanstalk:cloudwatch:logsStreamLogs=true(plusRetentionInDays), and correct the path. -
The EB
psrow needs enhanced health —generate-docs.md:1386. Per the CLI help,describe-environment-health"is only available with AWS Elastic Beanstalk Enhanced Health".SystemTypedefaults tobasic. Fix:aws elasticbeanstalk describe-environments --environment-names {{EB_WEB_ENVIRONMENT}} --query 'Environments[].[EnvironmentName,Status,Health]' --output table. -
The config rows use an SSM path nothing creates or reads, so
config:seton a new key is a silent no-op —generate-docs.md:1383-1385,:1405-1407.
- On EB, the instance role can read only
parameter/${var.project_name}/*, andproject_nameis not always{{app_name}}. EB also injects only parameters named in anenvironmentsecretssetting. - On Fargate, the task definition has no
secretsblock, and the execution role has no SSM permissions.
Fix: use a generated prefix variable ({{CONFIG_PREFIX}} = /${project_name}/). Split the row into two cases:
- change an existing key:
put-parameter, then restart or force-new-deployment; - add a key: add an
environmentsecretssetting or asecretsentry plusssm:GetParameters, thenterraform apply.
-
pg:psqlreads a secret the Terraform never creates —generate-docs.md:1392,:1414. The only secret generated is${project_name}-${environment}/db-credentials, holding JSON{username,password}. Fix: read that secret,jqthe fields intoPGPASSWORD, and connect withsslmode=verify-full. Add{{DB_SECRET_NAME}}to the variable table. -
EB
releasesuses the wrong application name —generate-docs.md:1389.aws_elastic_beanstalk_applicationisname = var.project_name. Fix: add{{EB_APPLICATION}}and use it here. -
Fargate
releases/rollbackdon't work with the guide's own:latestdeploy flow —generate-docs.md:1411-1412. Phase 3 pushes:latestand uses--force-new-deployment, so the revision never changes and old images become untagged. A CLI-registered revision is also replaced on the next apply. Fix: roll back by image digest, throughterraform.tfvarsandterraform apply. -
CLI scale is reverted by the next apply and bypasses
writers_quiesced—generate-docs.md:1387,:1409.desired_count = var.writers_quiesced ? 0 : …and the EB Min/MaxSize settings are Terraform-managed. Fix: add a note under both tables:
- persistent scale or rollback means editing
terraform.tfvarsand runningterraform apply; - don't run the scale, restart or config:set rows while
writers_quiesced = true.
- [PR honesty] The closing paragraph points at a section that exists only in open #612 —
generate-docs.md:1423("Developer workflow section ofmigration-report.html"). Fix: land after #612 and say so in the body, or reword so the line doesn't depend on the report.
Nits
generate-docs.md:1373: "nothing here needs to be built" holds only once Should-fix 1-4 are done.generate-terraform.md:1180: the policy isn't "below";aws_iam_role_policy.ecs_task_execis insecurity.tf.generate-docs.md:1395: usehttp://for the EB CNAME (Phase 4 does).openis macOS-only.generate-docs.md:1393/1415: on the Aurora branch, a snapshot iscreate-db-cluster-snapshot. Whethercreate-db-snapshotrejects an Aurora member is unknown. These rows render even when there is no Postgres; consider{{IF has_postgres}}.generate-docs.md:1391/1413: maintenance mode changes one listener, but both:80and:443exist.generate-docs.md:1403:--command bashfails on slim/alpine images; mentionsh.
Heads-up (out of diff, pre-existing — not blocking)
generate-docs.md:862-865"Deploy to Fargate" uses{{app_name}}-cluster, but the Terraform name is${project_name}-${environment}, so the guide now shows two cluster names.generate-docs.md:821: the manual EB deploy has the same application-name mismatch as Should-fix 6.- On multi-app inventories, the ECS service, log group and EB environment names don't include the app, so "repeat per app" resolves to colliding names.
- With #614: on its buildpack path the ECR repo is
{{project_name}}/{{app_name}}, tagged by{{git_sha}}. Once both land, the Fargatereleases/rollbackrows should follow that.
Alternatives (max 2)
- ECS Exec behind a variable (
enable_ecs_exec, defaulttrue). Production owners who don't want interactive shells can turn it off. Follow-up. - Express scale and rollback as
terraform.tfvarsedits. This avoids drift and respectswriters_quiesced. Recommendation: switch those rows (Should-fix 8), and keep the CLI for read-only rows.
Verdict
Comment. One Blocking finding (the policy gate fails on every Fargate run) and nine Should-fix items are open. CI is green and the change is well scoped, but several rows don't work against the stack the skill generates.
Commenting rather than approving: author == reviewer identity.
…r the generated stack Review findings on PR aws#615. - beanstalk.tf attaches AmazonSSMManagedInstanceCore to the EB instance role; the `run bash` / one-off rows use `aws ssm start-session` against the environment's instance plus `docker exec`, and `eb ssh` is dropped (no EC2KeyName, no :22 ingress; AWSElasticBeanstalkWebTier has no ssm). - config:set is split into "existing key" and "new key" on both targets. A new key is a silent no-op until Terraform binds it: EB injects only environmentsecrets names and its role reads only parameter/<project>/* and secret:<project>/*; the Fargate task definition emits no secrets block and the execution role has no SSM / Secrets Manager read. The rows name the {{PROJECT_NAME}} prefix and the Terraform change. - EB rows use {{EB_APPLICATION}} (var.project_name) instead of the Heroku app name; `ps` stays on describe-environments (basic health). - ps:scale / restart rows note that the next apply reverts a CLI scale, point at terraform.tfvars for a persistent change, and forbid scaling or restarting while writers_quiesced = true. - pg:psql reads {{DB_CREDENTIALS_SECRET}} ({username,password} JSON) with jq into PGPASSWORD and connects with sslmode=verify-full; falls back to db_username / db_password in terraform.tfvars when RDS Proxy is absent. - Fargate rollback is by image digest via container_image_* in terraform.tfvars (Phase 3 pushes :latest with --force-new-deployment, so there is no previous tag); the releases row lists imageDigest. - compute.tf comment points at aws_iam_role_policy.ecs_task_exec in security.tf (Step 5) instead of "below". - The closing paragraph no longer depends on the "Developer workflow" report section from another PR. - New good-heroku-fargate-ecs-exec policy fixture (count 31 -> 32); the cheat-sheet test file pins every row above. Verification: `mise run ci` passed (lint:md 0 errors, lint:artifacts, lint:vendored-parity, lint:skill-refs, pytest 1490 passed + 225 subtests, telemetry 311 passed); test_validate_terraform_policy.py and test_heroku_command_cheatsheet.py 78 passed together.
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖] Follow-up reviewed at f36a765b0e07911c6cbf4dd83be72b2dad95fee8. The eight original failure cases have targeted fixes, but the replacements introduce seven P2 defects; one additional P2 case was missed in my first review. The complete correction list is:
- Restrict the IAM exception to a fully literal allowlisted action list.
- Use
compute.tffor the ECS/task/ALB file references. - Convert the EB load-balancer name to an ARN before listener lookup.
- Use shell syntax supported by the advertised
/bin/shsession. - Give the documented database session a valid credential-retrieval path.
- Handle the default Classic Load Balancer maintenance case — my prior miss.
- Validate custom/saved retention against EB's accepted values.
- Resolve the
openhostname for the app being rendered.
Details and acceptance criteria are in the follow-up replies and inline maintenance comment. Local pytest: 161 tests and 121 subtests passed. Markdown lint (four files), vendored parity, reference checks and git diff --check passed. Adverse policy and shell probes reproduce the listed failures despite those passing suites. No live AWS deployment was performed.
… command cheat sheet Review findings on PR aws#615. - validate-terraform-policy.py: the ssmmessages exemption now requires every Action list member to be a quoted literal from the allowlist; an expression member (local.x, var.y, function call) keeps the statement under no_wildcard_iam. Rule docs updated (SKILL.md, security-posture-rules.md). - generate-docs.md cheat sheet: Fargate rows point at terraform/compute.tf (never ecs.tf / alb.tf); pg:psql reads the db-credentials secret on the operator workstation (neither session role can read it) with POSIX-portable `jq --argjson` instead of a bash here-string, and the no-RDS-Proxy fallback stays explicit; `open` resolves the rendered app's own hostname through dns_hostnames[].heroku_app ({{APP_HTTPS_HOSTNAME}}) instead of the global first custom domain; EB maintenance:on branches on the actual load balancer (Classic default outside has_route53_dns -> maintenance application version; ALB -> name -> ARN -> listeners). - beanstalk.tf RetentionInDays is mapped to the smallest EB-accepted value >= operational.log_retention_days (documented in the template, Q13 and the assemble checklist so reused preferences are validated the same way). Tests: test_ssmmessages_exception_requires_every_member_to_be_a_literal; test_terraform_file_references_match_the_emitted_inventory; test_pg_psql_credential_path_is_the_operator_not_the_session_role (dash -n); test_eb_maintenance_row_branches_on_the_actual_load_balancer_type; test_open_hostname_is_the_rendered_apps_own_dns_hostname; test_eb_retention_is_mapped_to_the_accepted_domain. Verified: `mise run ci` passed (lint:md 0 errors, lint:artifacts, lint:vendored-parity, lint:skill-refs, pytest 1495 passed + 225 subtests, telemetry 311 passed).
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖] Completed the follow-up at dd306b7b4020d6ac84ef085dba3f75028bea3b5e. All eight findings from the previous round are verified fixed. Three separate issues remain: one P1 permission expansion and two P2 command prerequisites. They existed at the previous reviewed head and were missed in my earlier review; they were not introduced by this latest fix.
The consolidated correction list is: preserve the EB parameter-read boundary when composing the Session Manager policy; cover every forwarding EB listener during maintenance; and provision the client CA file on the actual host running psql for both migration approaches. The IAM details are inline, and the maintenance/database details are in the original conversations. The correction paths are the two generation fragments and the command-contract tests.
Local validation: 167 tests and 121 subtests passed; all six changed Markdown files passed lint; vendored parity, reference checks, and git diff --check passed. The independent review covered all ten entries and their unchanged consumers. Passing tests do not check the three external contracts above. No live AWS access, decryption, maintenance request, or database handshake was performed.
Replace AmazonSSMManagedInstanceCore with a Session Manager inline policy so parameter reads stay under the project prefix; switch every forwarding EB ALB listener for maintenance mode; fetch the RDS CA bundle on the psql host in both pg:psql rows. Verified: mise run ci passed (1497 tests).
Problem
Most of what Heroku developers do every week (
logs --tail,run bash,config:set,ps:scale,releases,rollback,maintenance:on,pg:psql) has a direct AWS equivalent, but the generatedMIGRATION_GUIDE.mdnever says what it is. The guide covers provisioning, data migration, cutover and rollback, and then stops. A developer-experience-sensitive customer reads that as "and then you're on your own", when the honest answer for these commands is "same capability, different command".Fix
A new
MIGRATION_GUIDE.mdsection, Day-to-day commands: Heroku → AWS, placed after Decommission and before Manual Migration Items, with a TOC entry. One table per compute target present in the design (Elastic Beanstalk, ECS Fargate), one row per Heroku command, AWS side filled in with the app's generated names ({{EB_RUN_ENVIRONMENT}},{{EB_APPLICATION}},{{ECS_CLUSTER}},{{app_name}}), so the commands can be pasted. Covered: logs,run bash, one-off tasks, config list/get/set,ps,ps:scale,restart,releases,rollback,maintenance:on/off,pg:psql,pg:info/pg:backups:capture,addons,open. A closing paragraph names the two commands that have no one-liner (git push heroku→ Phase 3 workflow;pipelines:promote→ not generated) and suggests thebin/wrapper scripts as a first platform-team task.To make the
heroku run bashrow true for Fargate, the generatedecs.tfnow setsenable_execute_command = trueon each service and adds the fourssmmessages:*channel actions to the task role (ECS Exec); thetf-best-practicespolicy validator exempts exactly that statement shape (with tests for the mixed cases). To make the Elastic Beanstalkrun bashrow true,beanstalk.tfattachesAmazonSSMManagedInstanceCoreto the EB instance role soaws ssm start-sessionworks (the environment has no key pair or :22 ingress, soeb sshis not offered). The new template variables are documented in the Template Variable Resolution table.Scope
plugins/aws-startup-advisor/skills/heroku-to-aws/references/phases/generate/generate-docs.mdandgenerate-terraform.mdonly. No root or.githubchanges. Independent of the other open heroku-to-aws PRs for the generated commands; the closing paragraph points at the migration report generically rather than at the "Developer workflow" section #612 adds, so #612 is a soft dependency for that one sentence only (its "Covered — different command" rows now have a concrete destination in the guide).Known limitations: the
maintenance:onequivalent is a manual ALB listener change, not a generated Terraform toggle. ECS Exec also requires the Session Manager plugin on the developer's machine, noted in the row. Commands were checked against current AWS CLI syntax but not executed against a live estate in this PR.Type of Change
Select at least one.
Startup Advisor review checklist
By submitting this PR, I confirm that:
mise -C plugins/aws-startup-advisor run ciand all checks pass.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.