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

build(docker): simplification of assembly to facilitate further support #24504

Merged
merged 1 commit into from
Jun 24, 2023
Merged

build(docker): simplification of assembly to facilitate further support #24504

merged 1 commit into from
Jun 24, 2023

Conversation

alekseyolg
Copy link
Contributor

@alekseyolg alekseyolg commented Jun 23, 2023

During the build of the image, a lot of unnecessary duplicate commands are passed, which are redundant. This PR allows you to correct this oversight and allows you to facilitate the process of further support by developers.
Added parameter DOCKER_BUILDKIT=1 to add new features to build the image.
Also in the docker-bootstrap.sh script, the if function has been replaced with a case - it looks much better and is easier to maintain. "Unknown Operation!!!" was added to make it clear to users in case of image customization what their mistake is.

TESTING INSTRUCTIONS

Manually build from the Dockerfile,

DOCKER_BUILDKIT=1 docker build -t superset

And the build should finish without error.

Successfully tagged superset:latest

@@ -50,7 +50,7 @@ jobs:
mkdir -p ./build
echo ${{ github.sha }} > ./build/SHA
echo ${{ github.event.pull_request.number }} > ./build/PR-NUM
docker build --target ci -t ${{ github.sha }} -t "pr-${{ github.event.pull_request.number }}" .
DOCKER_BUILDKIT=1 docker build --target ci -t ${{ github.sha }} -t "pr-${{ github.event.pull_request.number }}" .
Copy link
Contributor

Choose a reason for hiding this comment

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

The runner already uses docker-buildx by default. Have a look here. So I don't think it is necessary setting this variable DOCKER_BUILDKIT=1 explicitly. Please correct me if I am wrong.

Copy link
Contributor Author

@alekseyolg alekseyolg Jun 24, 2023

Choose a reason for hiding this comment

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

@sebastianliebscher Hello!
Originally my code didn't include this, but without it, an error pops up that this variable is not set and a build error occurs.
Apparently this feature is not used by default.

@craig-rueda craig-rueda merged commit ba3bdc0 into apache:master Jun 24, 2023
@sebastianliebscher
Copy link
Contributor

@craig-rueda @alekseyolg
GitHub runners use buildx. The tool buildx is a "Docker CLI plugin for extended build capabilities with BuildKit". So there is no need to explicitly set DOCKER_BUILDKIT=1.

@alekseyolg alekseyolg deleted the patch-1 branch June 24, 2023 18:54
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 8, 2024
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 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/L 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants