feat(issue-1236): enhance docker build with additional build configuration#1284
Conversation
…ation - allow buildContextPath argument to change build context path - allow custom docker build arguments
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The CLI-side wiring (schema, dev server, local packager) is clean and well-tested.
However, I think there is one blocking correctness issue: the new buildContextPath and customDockerBuildArgs fields are not honored by agentcore deploy — only by agentcore dev and the local agentcore package validation. The deploy path goes through the CDK construct (@aws/agentcore-cdk → AgentCoreApplication → ContainerSourceAsset + ContainerBuildProject), which:
- Uploads only
codeLocationas the source asset (sobuildContextPathoutsidecodeLocationis impossible — files referenced in the Dockerfile won't be in the upload). - Runs a hard-coded buildspec:
docker build -t $IMAGE_URI -f $DOCKERFILE_PATH .— no--build-argflags, no overridable context. - Does not include
buildContextPath/customDockerBuildArgsin its ownAgentEnvSpecschema, so even if the CLI sends them, zod will strip them on the construct side.
The net result is that a user following the new "Shared Dockerfile (monorepo)" docs example will see it work locally with agentcore dev and agentcore package, but agentcore deploy will silently use the wrong context (just codeLocation) and no build args, producing either a broken image or a deploy-time docker build failure with a confusing error.
See inline comments for specifics and options to address this. Other comments are smaller follow-ups.
| const buildResult = spawnSync( | ||
| runtime, | ||
| ['build', '-t', imageName, '-f', dockerfilePath, ...getUvBuildArgs(), codeLocation], | ||
| ['build', '-t', imageName, '-f', dockerfilePath, ...getUvBuildArgs(), ...buildArgFlags, buildContext], |
There was a problem hiding this comment.
Blocking — feature not honored on agentcore deploy.
ContainerPackager is only used by agentcore package (local validation) and agentcore dev. The actual deploy path goes through the CDK construct (@aws/agentcore-cdk), where:
ContainerSourceAsset(inagentcore-l3-cdk-constructs) uploads onlyresolveCodeLocation(spec.codeLocation, configRoot)as the source asset —buildContextPathis never read, so files referenced from outsidecodeLocationwon't be uploaded.ContainerBuildProject's buildspec hard-codesdocker build -t $IMAGE_URI -f $DOCKERFILE_PATH .with no--build-argplumbing.- The L3 construct's own
AgentEnvSpecschema doesn't include these new fields, so zod will strip them on the construct side anyway.
This means the documented "Shared Dockerfile (monorepo)" workflow in docs/container-builds.md will work locally and silently break (or behave wrongly) on deploy. That is a serious foot-gun.
Options to address:
- (Preferred) Land a paired change in
aws/agentcore-l3-cdk-constructsfirst that:- Adds
buildContextPathandcustomDockerBuildArgstoAgentEnvSpecSchema. - Updates
ContainerSourceAssetto uploadbuildContextPath(when set) instead ofcodeLocation, and adjusts the dockerfile path so it remains valid relative to that asset. - Updates
ContainerBuildProject's buildspec /ContainerImageBuilderto forward--build-argflags through CodeBuild env vars (e.g. via the custom resource properties, with the buildspec interpolating$BUILD_ARGS). - Bumps the construct dep here once published.
- Adds
- Hold this PR until that construct change is merged + published, and gate the schema additions in this repo on having the new construct version.
- As an interim, validate in the CLI's schema (or in the deploy command) that these fields are not set and reject with a clear "only supported for local dev/package" error — but that significantly waters down the feature and the docs example would have to be removed.
I'd lean towards option 1. Happy to help review the construct-side PR.
| } | ||
| ``` | ||
|
|
||
| The shared `Dockerfile` can then branch on the build arg: |
There was a problem hiding this comment.
Tied to the deploy issue: this example will not work end-to-end with agentcore deploy until the CDK construct side is updated. If we go with the "land construct first" path that's fine; otherwise this doc section should be reworded to make clear this is local-dev-only, or removed.
| "customDockerBuildArgs": { "AGENT_NAME": "agent-one" } | ||
| }, | ||
| { | ||
| "name": "agent-two", |
There was a problem hiding this comment.
Separate issue with this example regardless of the deploy plumbing: dockerfile is validated as a filename (no path separators — see src/schema/schemas/agent-env.ts and getDockerfilePath in src/lib/constants.ts), and getDockerfilePath joins it onto codeLocation. So the implied "single Dockerfile at the project root" setup isn't achievable today — each agent's codeLocation would still need its own Dockerfile file (even if it's just a one-liner that delegates).
Either (a) extend dockerfile to accept a path relative to the build context (and resolve it accordingly), or (b) update this section to clarify that each codeLocation must still contain a Dockerfile (which can simply be a thin wrapper / symlink) and adjust the example accordingly.
| * Useful for parameterising a shared Dockerfile per agent (e.g. `{ "AGENT_NAME": "myagent" }`). | ||
| * Container builds only. | ||
| */ | ||
| customDockerBuildArgs: z.record(z.string().min(1), z.string()).optional(), |
There was a problem hiding this comment.
Optional, but worth considering: the existing AgentEnvSpecSchema in the sibling construct repo already has a comment near this validator block (If adding more Container-specific fields, consider consolidating into a containerConfig object (see networkConfig pattern)). With this PR we're now up to three flat container-only fields (dockerfile, buildContextPath, customDockerBuildArgs) plus the cross-cutting validators. Grouping these under a single optional containerConfig: { dockerfile?, buildContextPath?, customDockerBuildArgs? } would make the cross-build-type validation simpler (build !== 'Container' && data.containerConfig becomes one check) and is more future-proof. Not strictly required for this PR, but if you agree it's worth doing it now before this ships.
Description
Add support for two new options in the runtime configurations for Docker-based builds :
buildContextPathcustomDockerBuildArgsBoth are optional and this change is backward-compatible.
Related Issue
Closes #1236
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.