Skip to content

feat(aws-startup-advisor): containerize Heroku buildpack apps with Cloud Native Buildpacks in heroku-to-aws - #614

Open
herosjourney wants to merge 8 commits into
aws:mainfrom
herosjourney:feat/heroku-cnb-build
Open

herosjourney wants to merge 8 commits into
aws:mainfrom
herosjourney:feat/heroku-cnb-build

Conversation

@herosjourney

@herosjourney herosjourney commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Most Heroku apps are built by buildpacks and have no Dockerfile. The skill's compute targets (Elastic Beanstalk Docker platform, ECS Fargate) run container images, so today MIGRATION_GUIDE.md tells the customer to write a Dockerfile per app from Procfile-to-Dockerfile patterns. For a large estate that is the single biggest line item in the migration, it changes how every app is built, and it is the first thing a developer-experience-sensitive customer points at. Separately, the skill's Fir-detection constraint said "no CNB buildpack configuration in ANY output", which blocked the obvious fix even for Cedar apps.

Fix

Containerize buildpack apps with Cloud Native Buildpacks and Heroku's own builder images, so the image is produced by the same buildpacks Heroku uses, from the same source, with no Dockerfile.

Clarify Q12e — Containerization method (fires when Q12b is buildpack_only or partial): CNB with the Heroku builder (recommended default) or write Dockerfiles. Written to preferences.operational.containerization_method; containerized apps get dockerfile without asking.

Generate (Terraform)

  • New ecr.tf when CNB is selected: one immutable-tag, scan-on-push repository per app with a keep-last-30 lifecycle policy. Every process type runs the same image with a different command.
  • beanstalk.tf: AmazonEC2ContainerRegistryReadOnly on the EB instance role and PORT=8080 as an environment property (Heroku injects PORT; a pack-built image does not).
  • deploy-eb.yml CNB variant: OIDC login, amazon-ecr-login, setup-pack, one pack build --publish per push, then one Dockerrun.aws.json bundle per EB environment with Image = the ECR tag and Command = the process type. Builder tag follows the app's stack: heroku-22 → heroku/builder:22 (deprecated upstream, noted), heroku-24 → :24, heroku-26 or unknown → :26 (Heroku's recommended builder). Verified against github.com/heroku/cnb-builder-images and the EB single-container Dockerrun.aws.json reference (Command/Entrypoint keys).
  • Fargate: var.<container_image_variable> points at the ECR repository and the task's command = ["<process_type>"].

Generate (docs): the Containerization Prerequisites section has a CNB branch (local pack build check, how process types map, PORT, release phase not yet automated, custom BUILDPACK_URLs need --buildpack or a Dockerfile for that app, immutable tags instead of heroku rollback); the Dockerfile branch is kept for containerization_method: dockerfile. Build-and-push for Fargate shows pack build --publish under CNB. File inventory lists ecr.tf.

Design: the Fir constraint is rescoped to what it was protecting: no ARM/Graviton targeting and no Fir builder project.toml. It explicitly does not cover CNB on x86 for Cedar apps. design.md's _assert text and the fir_generation_note wording updated to match.

Scope

plugins/aws-startup-advisor/skills/heroku-to-aws/ only: references/phases/clarify/clarify-interview.md, references/phases/clarify/clarify-assemble.md, references/phases/design/design-mapping.md, references/phases/design/design.md, references/phases/generate/generate-terraform.md, references/phases/generate/generate-docs.md. No root or .github changes. Independent of #608–#611. #612 and #615 have merged; this PR is merged up to them (the CNB ECR read-only attachment sits next to #615's scoped Session Manager policy). Follow-up after this lands: #612's developer-workflow table should change its push-to-deploy row to "Generated" for CNB apps (one line).

Known limitations: the generated workflow builds and deploys but does not yet run the release process type before traffic shifts, and has no promote or review-app jobs (next PR). Heroku builder images are AMD64 (and ARM64 for 24/26); the skill does not emit Graviton targets, so x86 is assumed. Apps using private or custom buildpack URLs are called out as needing --buildpack flags or a Dockerfile. The CNB path has not been exercised end-to-end against a real estate in this PR; the pack/ECR/Dockerrun mechanics are standard and documented, but a fixture run with a buildpack app would be the right follow-up.

Type of Change

Select at least one.

  • New skill
  • Bug fix
  • Documentation update
  • CI/CD change
  • Other

Startup Advisor review checklist

By submitting this PR, I confirm that:

  • I have reviewed the full diff and understand all changes I am submitting.
  • I have tested my changes and confirmed they work as intended.
  • I have run mise -C plugins/aws-startup-advisor run ci and all checks pass.
  • I have disclosed any known issues, limitations, or untested scenarios.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…oud Native Buildpacks in heroku-to-aws

Most Heroku apps have no Dockerfile. The skill told customers to write one
per app. Use Heroku's own CNB builder images instead, so the image is built
by the same buildpacks from the same source.

- Clarify Q12e (when buildpack_only/partial): CNB with the Heroku builder
  (default) or Dockerfiles -> preferences.operational.containerization_method.
- Terraform: ecr.tf (one repo per app) when CNB; EB instance role gets ECR
  read-only; PORT=8080 environment property; deploy-eb.yml CNB variant builds
  once with pack --publish and deploys one Dockerrun.aws.json bundle per EB
  environment with Command = process type. Builder tag follows the app's
  stack (22/24/26; 26 is Heroku's recommended builder). Fargate tasks point
  at the ECR repo with command = [process_type].
- Docs: CNB branch of Containerization Prerequisites (local pack check,
  process-type mapping, PORT, release phase caveat, custom buildpack URLs,
  immutable tags); Dockerfile branch retained; pack build in the Fargate
  build-and-push steps; ecr.tf in the file inventory.
- Design: the Fir constraint is rescoped to ARM/Graviton and Fir builder
  project.toml; CNB on x86 for Cedar apps is explicitly allowed. design.md
  _assert and fir_generation_note wording updated.

Verified builder images against github.com/heroku/cnb-builder-images and the
Dockerrun.aws.json v1 Command/Entrypoint keys against the EB docs.

@leon1418 leon1418 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[🤖 AI review 🤖] Reviewed head 01386cdb5c3917ee2cd5ac2bb45a5bf51c021c3f with OCR-assisted full Git coverage, an isolated independent general reviewer, and parent verification.

The CNB path needs the corrections below before it is usable. The required artifact checker rejects the generated workflow, the setup-pack revision does not exist, and CMD does not select CNB processes.

Consolidated correction list:

  1. Make the generated CNB workflow pass the mandatory artifact checker without weakening its rejection of unsupported shell.
  2. Pin an existing setup-pack release revision.
  3. Use process-specific entrypoints consistently in deployment and local examples.
  4. Align EB and Fargate PORT settings, mappings, documentation and gates.
  5. Support or explicitly prevent CNB with the offered CodePipeline/manual methods.
  6. Preserve legacy preference defaults and make the documentation checklist match CNB output.
  7. Make retries compatible with immutable ECR tags.
  8. Preserve existing Dockerfile apps in partial estates.
  9. Honor or explicitly resolve the external-registry choice.

Validation: 1,475 plugin tests and 225 subtests passed; the independent reviewer also ran 83 generated-artifact tests. Markdown lint, vendored parity, skill-reference checks and git diff --check passed. Separately, the extracted CNB workflow fails the required artifact checker while base/Dockerfile controls pass. GitHub CI is green; no live AWS deployment or real pack image build was performed.

…ker-clean and process-correct

Review findings on PR aws#614.

- CNB deploy-eb.yml: split into build / Dockerrun-bundle / deploy steps so the
  elasticbeanstalk step is a shape validate-migration-artifacts.py verifies
  (plain ${VAR} interpolation, literal for-loop over EB_PROCESS_TYPES, no
  heredoc, no ${X%%=*}, no cd/mktemp). Pin setup-pack@v6.2.2 (full tag that
  exists and contains setup-pack/action.yml; no moving @v5/@v6 tag is published).
- Select CNB process types through the entrypoint (/cnb/process/<type>):
  Dockerrun Entrypoint without Command, ECS entryPoint, EKS command, local
  docker run --entrypoint examples.
- One port per EB web app: drop the hardcoded PORT=8080 EB setting, keep
  var.eb_application_port_<app>_web, carry it into the workflow as EB_WEB_PORT /
  Dockerrun ContainerPort; Fargate CNB web containers get PORT=8080 matching
  their port mapping.
- Immutable ECR tags: build step checks `aws ecr describe-images` and reuses
  the commit's image on reruns; version labels stay unique per run attempt.
- Resolve the containerization method per app (stack == container keeps its
  Dockerfile; buildpack apps use the chosen method) in Terraform and docs.
- CNB + codepipeline: Q12d withholds the option, Generate halts on stale
  preferences; manual deploy gets a pack build + Dockerrun bundle variant.
- CNB + external registry: Q12 rejects it, Generate halts, ecr.tf/ECR login
  never emitted for external; checklist line added in clarify-assemble.
- Docs: default an absent containerization_method to dockerfile, rename the CNB
  heading to "Containerization Prerequisites: Cloud Native Buildpacks", and
  update the documentation checklist.

Tests: tests/test_migration_artifacts.py extracts the exact generated CNB and
Dockerfile workflows and validates them with require_eb_workflow=True, and
asserts the merged heredoc/loop shape still fails workflow_run_unverified.
Verified: `mise run ci` (lint:md, lint:artifacts, lint:vendored-parity,
lint:skill-refs, pytest 1478 passed / 227 subtests, telemetry 311 passed).

@herosjourney herosjourney left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed: head 01386cd (base 00840a0), 6 files (+234/−6). Only heroku-to-aws is touched, in the single plugin tree plugins/aws-startup-advisor.

Ran:

  • pytest plugins/aws-startup-advisor/skills/heroku-to-aws: 112 passed.
  • pytest plugins/aws-startup-advisor/tests: 1475 passed, 225 subtests.
  • Validator probe: I rendered the CNB-variant workflow from generate-terraform.md and ran it through tf-best-practices/scripts/validate-migration-artifacts.py with require_eb_workflow=True, which generate.md passes for EB + GitHub Actions. Results:
  • As shipped: ARTIFACTS_FAIL (eb_deploy_missing, plus workflow_run_unverified on the heredoc).
  • Heredoc → printf: still FAIL (unresolved loop list).
  • Bundle step split from literal per-environment deploy steps: ARTIFACTS_OK.
  • git ls-remote on the action refs: buildpacks/github-actions has v6.2.1 and v5.12.5 but no v5 or v6 ref. amazon-ecr-login@v2 and configure-aws-credentials@v4 resolve.

GitHub CI is green at head (startup-advisor, CodeQL, author checklist); it is not restated here.

Primary sources (read 2026-11-10):

  • heroku/cnb-builder-images README
  • buildpacks.io "Run the app" (specify-launch-process)
  • buildpacks/spec platform.md (launcher, exporter ENTRYPOINT)
  • heroku/buildpacks-procfile README
  • buildpacks.io pack build reference
  • AWS EB single-container-docker-configuration and docker-configuration.remote-repo
  • AWS ECR image-push-iam

Did not run:

  • A pack build or docker run (pack is not installed).
  • Live AWS calls, terraform validate, or a GitHub Actions run.
  • Not verified: how EB handles two setting blocks with the same namespace and name.

Cross-plugin parity: n/a. Single tree; no vendored files touched.

Merge state: clean into main. It shares files with #608, #609, #611, #612 and #615, with no textual conflicts.

Change narrative:

  • Problem: buildpack apps have no Dockerfile, and the guide tells users to hand-write one per app.
  • Mechanism: a new Clarify question, Q12e (containerization_method), defaulting to pack build with heroku/builder:{22,24,26}; one ECR repo per app; EB Dockerrun.aws.json bundles; the Fir constraint narrowed to Fir only.
  • Body vs diff:
  • "the skill does not emit Graviton targets" is not accurate: design-mapping.md:66-71 maps formations to arm64 when workshop.cpu_architecture == "arm64".
  • The EB Command key is cited as verified, but it is the wrong key for a buildpack image (Blocking 1).
  • The author discloses that the CNB path was not run end to end.
Axis Stance One line
Parsimony advisory Sized to the problem, but the new workflow is a hand-written shell program in a shape the repo's validator does not accept.
Completeness finding The default EB + GitHub Actions route fails the blocking artifact gate. The codepipeline/manual and all-Fargate routes are not wired for CNB.
Security advisory ECR repo meets tf-best-practices ECR rule 3 (scan_on_push) and is IMMUTABLE. The OIDC role's ECR permissions are incomplete and mis-scoped (Should-fix 3).
Alternatives advisory Two below.
Contradictions finding Duplicate PORT on web environments. The section name differs between the template, Step 5 and Q12e. "x86" is assumed, but the arm64 workshop path exists.
Overlap advisory The process-type launch rule is stated three times (Q12e, docs, Terraform), all wrong in the same way. State it once.
Cross-plugin parity clear Single tree.
Propagation & data finding generate-docs.md never extracts containerization_method and uses 3 template variables nothing produces.

Blocking

  1. [Generated artifacts] Non-web process types would start the web process.

Where:

  • generate-terraform.md:1665 ("Command": "${PROCESS}") and :1550 (ECS command)
  • generate-docs.md:151, :155-157
  • clarify-interview.md:793

Why it fails:

  • The CNB platform spec says the exporter sets ENTRYPOINT=/cnb/process/<default-process>, and the launcher "MUST append any user-provided <args> to process arguments".
  • buildpacks.io: "To run a non-default process type, set the process type as the entrypoint".
  • EB: "If you specify an Entrypoint, then Command is added as an argument to Entrypoint."

Consequence: every worker, clock or custom environment and every Fargate task runs a second copy of web, so background jobs never run and nothing reports an error.

Fix:

  • Dockerrun: "Entrypoint": "/cnb/process/${PROCESS}", and drop Command.
  • Fargate: entryPoint = ["/cnb/process/<process_type>"], and drop command.
  • Docs: docker run --rm --entrypoint worker {{app_name}}:local.
  • Regression: assert the CNB block contains /cnb/process/ and no "Command":.
  1. [Completeness] The CNB workflow fails Generate's blocking artifact gate on the default route — generate-terraform.md:1656-1677. generate.md:178-188 passes --require-eb-workflow for EB with eb_deploy_method github_actions or absent, and "Only exit 0 with ARTIFACTS_OK permits continuation". Q12e, Q12d and the EB compute target all default onto this route.

As shipped the block returns ARTIFACTS_FAIL. The heredoc, ${PAIR%%=*}, $(mktemp -d) and assignments inside the loop all fall outside the validator's supported shape, so Generate halts with GATE_FAIL for typical buildpack apps on Elastic Beanstalk.

Fix: write the bundles in a step that does not deploy (for PROCESS in <types>; do printf '{…"Entrypoint":"/cnb/process/%s"}' … > bundle-${PROCESS}/Dockerrun.aws.json; zip; done). Then add one literal create-application-version + update-environment step per generated EB environment, and drop EB_TARGETS. That shape returned ARTIFACTS_OK in the probe.

Regression: test_cnb_eb_workflow_passes in tests/test_migration_artifacts.py, mirroring test_source_eb_workflow_with_literal_environment_list_passes. The existing test only reads the first yaml block, which is why CI stayed green.

  1. [Generated artifacts] buildpacks/github-actions/setup-pack@v5 cannot be resolved — generate-terraform.md:1646. git ls-remote lists v5.12.5 and v6.2.1 but no v5/v6 tag or branch, so the first push fails with "Unable to resolve action". Fix: setup-pack@v6.2.1, or pin a commit SHA.

  2. [Contradictions] Two PORT settings on every EB web environment — generate-terraform.md:1336-1342 (new, PORT=8080) vs :1434-1440 (existing, var.eb_application_port_<app>_web, same resource). The new block is gated only by a prose comment, not {{IF}}.

Consequence: if the user's port wins, the buildpack app binds to it while nginx proxies to ContainerPort 8080, and every request returns 502.

Fix:

  • Wrap the new block in # {{IF containerization_method == "cnb_heroku_builder"}}.
  • Change the existing gate to process_type == "web" AND containerization_method != "cnb_heroku_builder".
  • Under CNB, drop the eb_application_port_*_web variable, its tfvars line and its docs row.
  1. [Completeness] CNB + codepipeline or manual deploys a bundle with neither a Dockerfile nor a Dockerrun — generate-terraform.md:1295. Only the GitHub Actions workflow gets a CNB variant. pipeline.tf deploys source_output directly, and the manual docs zip -r app.zip .. EB requires "a Dockerfile, a Dockerrun.aws.json file, or both."

Fix, either of:

  • Fall back to dockerfile on those routes, with warning cnb_requires_github_actions.
  • Add a CodeBuild pack stage, plus a manual pack build --publish + Dockerrun block.

Should-fix

  1. [Completeness] The all-Fargate CNB route is under-specified — generate-terraform.md:1520, :1548-1551.
  • ecr.tf sits inside Step 6.5, which is skipped when nothing is on EB.
  • There is no ecs.tf; Fargate goes in compute.tf.
  • A Terraform variable default cannot reference a resource.
  • The Fargate task sets no PORT, although docs :161 says it does.

Fix: move ecr.tf to its own step gated on cnb_heroku_builder, and emit image = "${aws_ecr_repository.<app>.repository_url}:${var.image_tag_<app>}", entryPoint and a PORT=8080 environment entry for web.

  1. [Contradictions] The arm64 workshop path would pull an amd64-only image — design-mapping.md:709. Lines 66-71 map EB to ec2_instance_type_arm64 and Fargate to ARM64 when workshop.cpu_architecture == "arm64". The workflow builds on ubuntu-latest with no --platform, and heroku/builder:22 is AMD64-only (cnb-builder-images README). The result is an exec format error at start. Fix: under CNB, keep x86_64 formations and warn with cnb_architecture_x86, or build with --platform linux/arm64 on builder 24/26.

  2. [Propagation] The OIDC role's ECR permissions contradict the ECR docs — generate-terraform.md:1693-1696. The push policy needs ecr:BatchGetImage too, and ecr:GetAuthorizationToken must be on "Resource": "*", not on the repository. generate-docs.md:837-839 still lists only EB permissions. Fix: correct the list and add a CNB branch to the docs role setup.

  3. [Propagation] generate-docs.md uses template variables nothing produces — :149 {{cnb_builder_tag}}, :888 {{git_sha}} / {{project_name}}. Step 0 (:43) never extracts containerization_method, so a preferences.json written before Q12e loses the Containerization Prerequisites section. Fix:

  • Extract containerization_method in Step 0, defaulting to "dockerfile".
  • Add table rows for cnb_builder_tag (from the app's stack), git_sha and project_name.
  1. [Contradictions] The section name differs between the template, Step 5 and Q12e — generate-docs.md:173 vs :2094, and clarify-interview.md:793-794. Under CNB the guide renders "Containerization: Cloud Native Buildpacks (no Dockerfile)", so Step 5's "Contains Containerization Prerequisites" check fails. Fix: make Step 5 branch on containerization_method, and align Q12e's wording.

  2. [Completeness] Under Q12b = partial, one global method also overrides apps that already have a Dockerfile — clarify-interview.md:770, :778. pack does not read a Dockerfile, so those apps' hand-tuned images are silently replaced. Fix: apps with a Dockerfile keep the dockerfile path.

Nits

  1. generate-terraform.md:1634: the example EB_TARGETS uses <app_name>-web, but beanstalk.tf names environments "${var.project_name}-<process_type>" (:1319).
  2. generate-terraform.md:1649: an IMMUTABLE ${GITHUB_SHA} tag makes "Re-run jobs" fail on push. Use ${GITHUB_SHA}-${GITHUB_RUN_ATTEMPT}, or skip the build when the tag exists.
  3. clarify-interview.md:154: Q12e sits between Q12b and Q12c, out of numbering order.

Heads-up (out of diff, pre-existing — not blocking)

  • design-mapping.md:709 "Do NOT produce ARM/Graviton instance targeting … in ANY output" contradicts :66-72, which already produces arm64 targets from the workshop. This PR now relies on the first.

Alternatives (max 2)

  1. Scope CNB to EB + GitHub Actions in this PR and fall back to dockerfile elsewhere, with warnings. This removes Blocking 5 and Should-fix 1 and makes the PR small enough to verify end to end. Recommendation: do this now; add Fargate/CodePipeline CNB as a named follow-up.
  2. One image per process type via pack build --default-process <type>. No entrypoint override would be needed, but it costs N builds and tags per push. Recommendation: keep a single image with Entrypoint: /cnb/process/<type>.

Verdict

Comment. On content this would be Request changes, with five Blocking findings:

  • the wrong launch key, so workers run web;
  • the workflow fails the blocking artifact gate on the default route;
  • an action ref that cannot be resolved;
  • a duplicate PORT;
  • no CNB deploy path for CodePipeline or manual.

Commenting rather than approving: author == reviewer identity.

…d compose the Fargate image from the repository

Review findings on PR aws#614.

- Move "Containerization method per app" and the ecr.tf section out of the
  EB-only Step 6.5 into a new Step 5.5 that runs for Fargate and Beanstalk
  alike, so an all-Fargate CNB plan gets its repositories; fix the ecs.tf
  reference (Fargate lives in compute.tf) and point cross-references at
  Step 5.5.
- Fargate CNB task definitions: image is
  "${aws_ecr_repository.<app_name>.repository_url}:${var.image_tag_<app_sanitized>}"
  (a variable cannot reference a resource); declare image_tag_<app_sanitized>
  in variables.tf and terraform.tfvars.example; the docs build step sets it.
- Architecture: Design pins CNB apps to x86_64 with warning
  cnb_architecture_x86 (heroku/builder images are linux/amd64 only and the
  workflow builds without --platform); Generate never emits ARM64 for them.
- OIDC role: ecr:GetAuthorizationToken on Resource "*", add
  ecr:BatchGetImage to the repository statement, and give the
  MIGRATION_GUIDE Phase 3 CNB branch the same two statements.
- Regression: test_cnb_ecr_and_fargate_image_do_not_depend_on_beanstalk_step.

Verified: `mise run ci` (lint:md 0 errors, lint:artifacts PASS, lint:skill-refs
PASS, pytest 1479 passed, telemetry 311 passed).

@leon1418 leon1418 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[🤖 AI review 🤖] Follow-up reviewed at 232714cba451385e50422da7eae1faa7cdac539f against the prior reviewed head 01386cdb5c3917ee2cd5ac2bb45a5bf51c021c3f.

Verified fixed: the mandatory artifact-checker failure, setup-pack revision, process entrypoints, EB web/Fargate port wiring, manual/CodePipeline handling, legacy guide defaults, immutable-image reuse, and the external-registry conflict. The partial-estate Dockerfile concern remains for prepared Dockerfiles on apps still using a Heroku buildpack stack; I replied in its original thread.

Consolidated remaining corrections (all P2):

  1. Preserve confirmed per-app Dockerfiles independently of the currently deployed Heroku stack.
  2. Make EB application-version labels reusable or unique across rerun attempts. The ECR fix is verified; the later label collision was an earlier miss, acknowledged in the original retry thread.
  3. Give worker-only EB bundles a valid port without requiring a web-only input.
  4. Apply the CodePipeline gate only to CNB apps actually deployed on EB.
  5. Make local CNB publication compatible with the x86 design. The ARM-host mismatch was another earlier miss.
  6. Define the initial ECR/image/Fargate provisioning order for the new required image-tag input.

Validation: 1,479 plugin tests and 227 subtests passed, including the new extracted-workflow regressions; the independent reviewer also ran the changed 74-test artifact suite. Markdown lint, vendored parity, skill-reference checks, and git diff --check passed. Exact shell probes reproduced the rerun and worker-only cases; a provider-free Terraform probe confirmed the missing-tag input failure. GitHub CI is green. No live AWS deployment, image build, or ARM runtime execution is claimed.

…s, amd64 builds and ECR bootstrap for the heroku-to-aws CNB path

Review findings on PR aws#614 (second round):

- Partial estates: Clarify Q12b gains a follow-up that stores the apps that
  already have a Dockerfile as operational.dockerfile_apps[] (added to the
  preferences.json machine contract); the per-app resolution in Step 5.5,
  generate-docs Step 0, Design's CNB architecture override and the Q12d /
  checklist CodePipeline gates use `stack == container OR in dockerfile_apps`,
  so a prepared Dockerfile on a buildpack-stack app is never sent through pack.
- Worker-only EB apps: the CNB bundles use EB_CONTAINER_PORT, which is the
  customer's eb_application_port_<app>_web for apps with an EB web environment
  and the documented 8080 constant for apps without one (workflow and manual).
- The clarify-assemble checklist rejects CNB + CodePipeline only when an actual
  EB app resolves to CNB (same predicate as Q12d and pipeline.tf), keeping the
  ECR requirement for CNB apps.
- Every publishing `pack build` passes --platform linux/amd64; the AMD64-only
  claim about builders 24/26 is corrected (they are multi-arch); the
  immutable-tag reuse checks the existing image's architecture with
  `docker manifest inspect` and fails with a clear error otherwise.
- image_tag_<app> defaults to "" with a two-phase bootstrap: the first apply
  creates the ECR repository and the Fargate service at desired_count 0
  (task image coalesces to the never-pulled `bootstrap` tag); Phase 1/3 of the
  guide and the variable description document the order.
- Version labels include GITHUB_RUN_ATTEMPT so a rerun after a failed
  update-environment is not rejected by create-application-version.

Verification: `mise run ci` passed (lint:md 0 errors, lint:artifacts PASS,
vendored parity, skill refs, 1483 pytest tests + telemetry). New regressions in
tests/test_migration_artifacts.py: worker-only CNB workflow passes the
--require-eb-workflow checker with a resolved port; amd64 flag and run-attempt
labels; dockerfile_apps accepted by validate-artifacts (string array) and
rejected as a string; image_tag variable extracted and probed with
`terraform plan -input=false` unset, set and invalid.

@leon1418 leon1418 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[🤖 AI review 🤖] Follow-up reviewed at f7fef57eca4eb2479f5ce6f20d7db06fc6fc3f56, against prior reviewed head 232714cba451385e50422da7eae1faa7cdac539f.

Verified fixed: EB rerun version labels, worker-only bundle ports, the per-EB-app CodePipeline gate, amd64 publication/reuse, and the first-time Fargate bootstrap. I replied with evidence in the original threads.

Four P2 corrections remain:

  1. Use canonical apps[].app_name when capturing and validating dockerfile_apps; the original Dockerfile-preservation concern remains open.
  2. Reconcile legacy partial-preference reuse with the new required-list gate.
  3. Supply the selected CNB build/publish and image/process handoff for all-EKS, or restrict that unsupported combination.
  4. List terraform/ecr.tf independently of EB presence in README.

Items 3 and 4 already existed at the previous reviewed head; I missed them in that review. Item 2 is introduced by the latest compatibility gate. The complete correction list is bounded to these affected contracts.

Validation: 1,483 plugin tests and 235 subtests passed; the independent reviewer also ran all 78 artifact-test cases. Markdown lint, vendored parity, skill references and git diff --check passed. Exact shell controls verified attempt-specific labels, process bundles and architecture rejection; provider-free Terraform controls verified empty/published/invalid tags and zero-task bootstrap. No live AWS deployment, real CNB image build or migration-agent generation was performed. CI was queued at the initial capture and is tracked separately.

Use apps[].app_name for dockerfile_apps, confirm Dockerfile status when reusing legacy partial preferences, add an EKS publish/image handoff for CNB apps, and list ecr.tf outside the EB-only README block.

Checks: mise run ci (lint:md, lint:artifacts, lint:skill-refs, pytest, telemetry) green.
leon1418
leon1418 previously approved these changes Nov 11, 2026
@herosjourney
herosjourney added this pull request to the merge queue Nov 11, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 11, 2026
Logan Kleier added 2 commits November 11, 2026 13:08
# Conflicts:
#	plugins/aws-startup-advisor/skills/heroku-to-aws/references/phases/clarify/clarify-interview.md
#	plugins/aws-startup-advisor/skills/heroku-to-aws/references/phases/generate/generate-terraform.md
@herosjourney

Copy link
Copy Markdown
Contributor Author

The four open findings are fixed and replied to, and the branch is merged up to current main (head 956ec2fe). Main now has the scoped Session Manager policy, so the CNB ECR read-only attachment sits next to it, and one test that expected a single managed policy now expects both. Full CI passed before the final merge from main. Requesting a fresh review of 956ec2fe; threads will be resolved once confirmed. Merge still needs one code-owner approval from someone who did not push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants