Skip to content

fix(shell): use bash -l only for buildpack apps#145

Open
ipmb wants to merge 1 commit into
mainfrom
non-login-bash
Open

fix(shell): use bash -l only for buildpack apps#145
ipmb wants to merge 1 commit into
mainfrom
non-login-bash

Conversation

@ipmb

@ipmb ipmb commented Jun 3, 2026

Copy link
Copy Markdown
Member

Docker/Dockerfile-based apps may set PATH in the image. Launching the shell with bash -l sources /etc/profile, which can clobber that PATH. Reserve bash -l for buildpack apps (where the CNB launcher wants login-shell semantics) and use plain bash otherwise.

Refactor shell task lookup along the way: memoize the DescribeTaskDefinition call with sync.Once, and split ShellTaskFamily into focused ShellTaskFamily and IsBuildpack accessors so callers no longer discard half the return.

Copilot AI 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.

Pull request overview

Adjusts interactive shell startup to avoid bash -l clobbering image-provided PATH for Dockerfile/Docker-based apps, while keeping login-shell semantics for buildpack-based apps. It also refactors shell task metadata lookup to avoid repeated ECS DescribeTaskDefinition calls.

Changes:

  • Use bash -l only for buildpack apps; use plain bash otherwise.
  • Memoize shell task definition lookup with sync.Once and cache task family/tags.
  • Split the previous combined API into ShellTaskFamily() and IsBuildpack() accessors.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
cmd/shell.go Chooses bash vs bash -l based on build system and uses IsBuildpack() for buildpack-specific exec wrapping.
app/app.go Caches shell task definition metadata via sync.Once; adds IsBuildpack() and simplifies ShellTaskFamily() return shape.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Docker/Dockerfile-based apps may set PATH in the image. Launching the
shell with `bash -l` sources /etc/profile, which can clobber that PATH.
Reserve `bash -l` for buildpack apps (where the CNB launcher wants
login-shell semantics) and use plain `bash` otherwise.

Refactor shell task lookup along the way: memoize the DescribeTaskDefinition
call with sync.Once, and split ShellTaskFamily into focused ShellTaskFamily
and IsBuildpack accessors so callers no longer discard half the return.

Add table-driven tests for IsBuildpack covering the build-system tag
detection and its backwards-compat defaults (missing/empty tag means
buildpacks).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ipmb ipmb force-pushed the non-login-bash branch from bb36b85 to 14edc00 Compare June 3, 2026 22:45
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.

3 participants