build: Parallelize the CI image builds (continued) #26698

mistercrunch · 2024-01-19T23:37:38Z

new approach

originally started from #25564, but got tangled in intricate changes around release tag management that conflicted with the approach used originally, so I started over by

refactoring scripts/docker_build_push.sh, that originally generated a bunch of docker build commands to be executed sequentially, here I move to this bash script running a single command given the parameters, so it can be reused to run multiple build commands in parallel while orchestrating in GitHub actions. I tried to DRY what was in there cautiously
changed GitHub actions to generate a matrix of commands for each "target" (dev, lean, lean39, lean310, ...) and "platform" (linux/amd64, linux/arm64), similar to build: Parallelize the CI image builds #25564

codecov · 2024-01-19T23:42:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f8f5c54) 69.48% compared to head (49a6f9b) 69.49%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #26698   +/-   ##
=======================================
  Coverage   69.48%   69.49%           
=======================================
  Files        1894     1894           
  Lines       74151    74151           
  Branches     8243     8243           
=======================================
+ Hits        51527    51528    +1     
+ Misses      20555    20554    -1     
  Partials     2069     2069

Flag	Coverage Δ
hive	`53.80% <ø> (ø)`
javascript	`56.86% <ø> (ø)`
mysql	`77.98% <ø> (+<0.01%)`	⬆️
postgres	`78.08% <ø> (ø)`
presto	`53.75% <ø> (ø)`
python	`83.02% <ø> (+<0.01%)`	⬆️
sqlite	`77.66% <ø> (ø)`
unit	`56.39% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dpgaspar

Thanks for recovering this!
just a comment

dpgaspar · 2024-01-22T11:22:47Z

.github/workflows/docker.yml

+        if: needs.config.outputs.has-secrets
+        uses: docker/login-action@v3
+        with:
+          username: ${{ secrets.DOCKERHUB_USERNAME }}


this is secrets.DOCKERHUB_USER

dpgaspar · 2024-01-22T11:41:15Z

.github/workflows/docker.yml


 on:
+  release:
+    types: [ published ]
  push:
    branches:
      - 'master'
  pull_request:
    types: [synchronize, opened, reopened, ready_for_review]


Just a comment: pull_request trigger will use the PR SHA and run any changes made on any PR. PRs from forks don't contain secrets

eschutho · 2024-01-22T18:30:56Z

.github/workflows/docker.yml

+            type=sha,prefix=,format=long
+            type=ref,event=pr
+            type=raw,value=master,enable={{is_default_branch}}
+            type=raw,value=latest,enable={{is_default_branch}}


@mistercrunch I updated the docker build at the same time that @sebastianliebscher was writing his initial PR so he didn't have a chance to capture the changes from this PR, but @sfirke and I had updated the docker image latest tag to point to the latest official release image only and not the latest production master image so that subsequent docker deployments would run from the latest official release instead of master. There's a script that you can use from the PR to get the value for latest, or feel free to update the way that runs, too.

mistercrunch · 2024-01-22T20:49:30Z

Ok, starting over and try a different approach

mistercrunch · 2024-01-22T23:40:15Z

Request for comments here, given this docker build matrix:

    strategy:
      matrix:
        target: ["dev", "lean", "lean310", "lean39", "websocket", "dockerize"]
        platform: ["linux/amd64", "linux/arm64"]
      fail-fast: false

Do we need every combination for each PR and each SHA in master? Seems a bit crazy. I get that people are on Apple hardware and that's why we need arm as a base platform, but 2 versions of python too? @craig-rueda @rusackas @dpgaspar

Dockerfile

eschutho · 2024-01-23T01:09:15Z

Request for comments here, given this docker build matrix:
    strategy:
      matrix:
        target: ["dev", "lean", "lean310", "lean39", "websocket", "dockerize"]
        platform: ["linux/amd64", "linux/arm64"]
      fail-fast: false
Do we need every combination for each PR and each SHA in master? Seems a bit crazy. I get that people are on Apple hardware and that's why we need arm as a base platform, but 2 versions of python too? @craig-rueda @rusackas @dpgaspar

We can probably remove lean39. I added that version as a request from someone in the community who later found a workaround, and it's probably not needed. We can maybe check with @rusackas on data around who is using it, but I don't think we really need to have docker images based on multiple versions of python.

eschutho

LGTM with or without the lean39 image. 👍

dpgaspar · 2024-01-23T08:58:02Z

Request for comments here, given this docker build matrix:
    strategy:
      matrix:
        target: ["dev", "lean", "lean310", "lean39", "websocket", "dockerize"]
        platform: ["linux/amd64", "linux/arm64"]
      fail-fast: false
Do we need every combination for each PR and each SHA in master? Seems a bit crazy. I get that people are on Apple hardware and that's why we need arm as a base platform, but 2 versions of python too? @craig-rueda @rusackas @dpgaspar
We can probably remove lean39. I added that version as a request from someone in the community who later found a workaround, and it's probably not needed. We can maybe check with @rusackas on data around who is using it, but I don't think we really need to have docker images based on multiple versions of python.

I may be missing some requirements, but totally agree, I actually don't see the point on building every flavour on every PR SHA, also on a PR from a fork, images are just built and not pushed so we are just testing them. My take would be to just leave lean, linux/amd64 and websocket, linux/amd64 on every PR SHA, should be enough for testing purposes then build push all of them when merging to master

michael-s-molina

LGTM

mistercrunch requested review from villebro, geido, eschutho, rusackas, betodealmeida, nytai, craig-rueda, john-bodley and kgabryje as code owners January 19, 2024 23:37

pull-request-size bot added the size/L label Jan 19, 2024

geido requested review from dpgaspar and michael-s-molina January 22, 2024 10:07

dpgaspar reviewed Jan 22, 2024

View reviewed changes

mistercrunch force-pushed the parallelize_image_builds_2 branch from 0c6ea6d to f4ad2ce Compare January 22, 2024 17:54

pull-request-size bot added size/XL and removed size/L labels Jan 22, 2024

eschutho reviewed Jan 22, 2024

View reviewed changes

mistercrunch force-pushed the parallelize_image_builds_2 branch from 2ea9bf9 to 237caa1 Compare January 22, 2024 20:09

pull-request-size bot added size/L and removed size/XL labels Jan 22, 2024

mistercrunch mentioned this pull request Jan 22, 2024

build: Parallelize the CI image builds (continued) #26697

Closed

mistercrunch force-pushed the parallelize_image_builds_2 branch from 9df9361 to f9096a7 Compare January 22, 2024 23:55

mistercrunch commented Jan 22, 2024

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

eschutho approved these changes Jan 23, 2024

View reviewed changes

dpgaspar approved these changes Jan 23, 2024

View reviewed changes

michael-s-molina approved these changes Jan 23, 2024

View reviewed changes

mistercrunch added 19 commits January 23, 2024 09:58

changing the approach

412a71d

typo

d9565fa

fix

83d2e7f

temp chec

5af0597

fix unit test

8864206

skipping --target for dockerize and websocket

ecc2705

missed a thing

d0784e7

missed a thing

b4f10b0

missed a thing

c8b707d

move DEV_TAG up

a989430

missed a thing

6bf409b

bumping actions versions

686c13b

use the right cache for ephemeral env

bee3014

trying v4

0182457

bring ephemeral build to top level

442520a

bring ephemeral build to top level

5d1b443

moving COPY after pip install

d40614e

fix dockerfile

5dff290

removing lean39 from matrix

49a6f9b

mistercrunch force-pushed the parallelize_image_builds_2 branch from 2ef8df8 to 49a6f9b Compare January 23, 2024 17:58

mistercrunch merged commit 363a8e6 into master Jan 23, 2024
63 checks passed

mistercrunch deleted the parallelize_image_builds_2 branch January 23, 2024 21:44

rusackas pushed a commit that referenced this pull request Jan 24, 2024

build: Parallelize the CI image builds (continued) (#26698)

af38706

eschutho pushed a commit to preset-io/superset that referenced this pull request Jan 31, 2024

build: Parallelize the CI image builds (continued) (apache#26698)

114ad9d

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

build: Parallelize the CI image builds (continued) (apache#26698)

0924e7b

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Parallelize the CI image builds (continued) #26698

build: Parallelize the CI image builds (continued) #26698

mistercrunch commented Jan 19, 2024 •

edited

Loading

codecov bot commented Jan 19, 2024 •

edited

Loading

dpgaspar left a comment

dpgaspar Jan 22, 2024

dpgaspar Jan 22, 2024

eschutho Jan 22, 2024 •

edited

Loading

mistercrunch commented Jan 22, 2024

mistercrunch commented Jan 22, 2024

eschutho commented Jan 23, 2024

eschutho left a comment

dpgaspar commented Jan 23, 2024 •

edited

Loading

michael-s-molina left a comment

build: Parallelize the CI image builds (continued) #26698

build: Parallelize the CI image builds (continued) #26698

Conversation

mistercrunch commented Jan 19, 2024 • edited Loading

new approach

codecov bot commented Jan 19, 2024 • edited Loading

Codecov Report

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Jan 22, 2024

Choose a reason for hiding this comment

dpgaspar Jan 22, 2024

Choose a reason for hiding this comment

eschutho Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

mistercrunch commented Jan 22, 2024

mistercrunch commented Jan 22, 2024

eschutho commented Jan 23, 2024

eschutho left a comment

Choose a reason for hiding this comment

dpgaspar commented Jan 23, 2024 • edited Loading

michael-s-molina left a comment

Choose a reason for hiding this comment

mistercrunch commented Jan 19, 2024 •

edited

Loading

codecov bot commented Jan 19, 2024 •

edited

Loading

eschutho Jan 22, 2024 •

edited

Loading

dpgaspar commented Jan 23, 2024 •

edited

Loading