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.

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

chore: removing redundant docker-entrypoint #17687

Merged

Conversation

ofekisr
Copy link
Contributor

@ofekisr ofekisr commented Dec 8, 2021

SUMMARY

Please review the docker-entrypoint.sh, it is redundant it does not really do something,
so you can set the default command in the image to running the server and can always override the CMD
and we earn preventing code reusing

note- think of the chart version, should it be incremented to 0.5.0 or 1.0.0 ?
the chart must use the new image

TESTING INSTRUCTIONS

let it build and run

@ofekisr ofekisr force-pushed the chore/removing_redundant_entrypoint branch from 25ca941 to b702822 Compare December 8, 2021 13:16
Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

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

LGTM

@amitmiran137 amitmiran137 merged commit aee5c9a into apache:master Dec 8, 2021
@amitmiran137 amitmiran137 deleted the chore/removing_redundant_entrypoint branch December 8, 2021 18:43
@Bytamine
Copy link

Bytamine commented Dec 8, 2021

helm chart uses tag latest (not a great thing), so old superset helm installations will fail with error
/bin/sh: 1: /usr/bin/docker-entrypoint.sh: not found
if the new image will be downloaded for some reason (pod restart or schedule to new node for example)

I upgraded helm chart from 0.3.10 to 0.5.0, this resolved the issue.

@@ -47,5 +47,5 @@ elif [[ "${1}" == "app" ]]; then
flask run -p 8088 --with-threads --reload --debugger --host=0.0.0.0
elif [[ "${1}" == "app-gunicorn" ]]; then
echo "Starting web app..."
/app/docker/docker-entrypoint.sh
/usr/bin/run-server.sh
Copy link

Choose a reason for hiding this comment

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

Having trouble starting docker composer due to this fix:

/app/docker/docker-bootstrap.sh: line 50: /usr/bin/run-server.sh: No such file or directory

Copy link
Member

Choose a reason for hiding this comment

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

Try running docker-compose pull first

Choose a reason for hiding this comment

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

macOS
/usr/bin/run-server.sh -> sh /app/docker/run-server.sh

@layoaster
Copy link

layoaster commented Jan 11, 2022

the image for Superset version 1.3.2 is not updated with these changes.

My values.yaml:

image:
  repository: apache/superset
  tag: "1.3.2"
  pullPolicy: IfNotPresent

What official image tag should I use to get the Superset version 1.3.2 working with this chart version ??

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants