Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: docker-compose-image-tag fails to start #31606

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+39 −7
Open

fix: docker-compose-image-tag fails to start #31606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
410ef9d
fix: docker-compose-image-tag fails to start
mistercrunch Dec 23, 2024
106c2e1
touching a file
mistercrunch Dec 23, 2024
23d1126
check for file changes
mistercrunch Dec 23, 2024
8aaeae9
change ordering of jobs
mistercrunch Dec 23, 2024
350a6f1
checkout
mistercrunch Dec 23, 2024
3a1a9ca
touchups
mistercrunch Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .github/workflows/docker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ concurrency:
cancel-in-progress: true

jobs:

setup_matrix:
runs-on: ubuntu-24.04
outputs:
Expand Down Expand Up @@ -103,3 +104,29 @@ jobs:
export SUPERSET_BUILD_TARGET=${{ matrix.build_preset }}
docker compose build superset-init --build-arg DEV_MODE=false --build-arg INCLUDE_CHROMIUM=false
docker compose up superset-init --exit-code-from superset-init

docker-compose-image-tag:
runs-on: ubuntu-24.04
steps:
- name: "Checkout ${{ github.ref }} ( ${{ github.sha }} )"
uses: actions/checkout@v4
with:
persist-credentials: false
- name: Check for file changes
id: check
uses: ./.github/actions/change-detector/
with:
token: ${{ secrets.GITHUB_TOKEN }}
- name: Setup Docker Environment
if: steps.check.outputs.docker
uses: ./.github/actions/setup-docker
with:
dockerhub-user: ${{ secrets.DOCKERHUB_USER }}
dockerhub-token: ${{ secrets.DOCKERHUB_TOKEN }}
build: "false"
install-docker-compose: "true"
- name: docker-compose sanity check
if: steps.check.outputs.docker
shell: bash
run: |
docker compose -f docker-compose-image-tag.yml up --exit-code-from superset-init
17 changes: 11 additions & 6 deletions docker/docker-bootstrap.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,12 @@

set -eo pipefail

# UV may not be installed in older images
pip install uv

# Make python interactive
if [ "$DEV_MODE" == "true" ]; then
echo "Reinstalling the app in editable mode"
uv pip install -e .
if command -v uv > /dev/null 2>&1; then
echo "Reinstalling the app in editable mode"
uv pip install -e .
fi
fi
Comment on lines 22 to 27
Copy link

Choose a reason for hiding this comment

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

Missing pip fallback in DEV_MODE category Functionality

Tell me more
What is the issue?

When UV is not available in DEV_MODE, the script silently skips the installation without falling back to regular pip or warning the user.

Why this matters

This could lead to missing dependencies in development environments where UV is not available, potentially causing runtime issues.

Suggested change

Add pip fallback:
if [ "$DEV_MODE" == "true" ]; then
echo "Reinstalling the app in editable mode"
if command -v uv > /dev/null 2>&1; then
uv pip install -e .
else
pip install -e .
fi
fi

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't need to reinstall in dev mode where uv is not present as it was the default install in that era

REQUIREMENTS_LOCAL="/app/docker/requirements-local.txt"
# If Cypress run – overwrite the password for admin and export env variables
Expand All @@ -35,7 +34,13 @@ if [ "$CYPRESS_CONFIG" == "true" ]; then
fi
if [[ "$DATABASE_DIALECT" == postgres* ]] ; then
echo "Installing postgres requirements"
uv pip install -e .[postgres]
if command -v uv > /dev/null 2>&1; then
# Use uv in newer images
uv pip install -e .[postgres]
else
# Use pip in older images
pip install -e .[postgres]
fi
fi
#
# Make sure we have dev requirements installed
Expand Down
2 changes: 1 addition & 1 deletion scripts/change_detector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
],
"docker": [
r"^Dockerfile$",
r"^docker/",
r"^docker.*",
],
"docs": [
r"^docs/",
Expand Down
Loading