FEATURE: print out compressed image sizes for amd64 #887
Conversation
Remove schedule jobs from running on forks, and remove forks from attempting to push on main update
featheredtoast
force-pushed
the
print-compressed-stats
branch
from
1a89e70
to
60328e7
Compare
This PR's print |
.github/workflows/build.yml
Outdated
| @@ -33,6 +34,7 @@ jobs: | |||
| echo "timestamp=$timestamp" >> $GITHUB_OUTPUT | |||
|
|
|||
| base: | |||
| if: (github.event_name != 'schedule' || github.repository == 'discourse/discourse_docker') | |||
There was a problem hiding this comment.
I'm not sure I follow here. Why are we skipping building of the base image when it is a scheduled job?
There was a problem hiding this comment.
skipping when it's a scheduled job and is a fork - this prevents a bunch of jobs on new forks until actions are disabled but will remove for now from the other comment.
.github/workflows/build.yml
Outdated
| - name: push to dockerhub | ||
| if: github.ref == 'refs/heads/main' | ||
| if: github.ref == 'refs/heads/main' && github.repository == 'discourse/discourse_docker' |
There was a problem hiding this comment.
I'm not sure if I agree here. I think forks should just disable actions if they don't want to run them rather than the main repository having to include this conditional.
There was a problem hiding this comment.
I'm of the opinion it's better for a fork to be able to get a passing CI run for experimenting. Forks may be interested in other jobs that are not push-to-discourse/base, such as test builds, running tests, and linting and we should consider allowing that.
We can remove it, but it was quite useful here for this work, so I thought to propose it.
.github/workflows/build.yml
Outdated
| # Push to local repo to compare sizes | ||
| docker run --quiet -d --rm -p 5002:5000 registry:2 | ||
| sleep 5 |
There was a problem hiding this comment.
Instead of doing this, I think we can follow https://docs.docker.com/build/ci/github-actions/local-registry/ instead.
There was a problem hiding this comment.
Oh didn't know services were available to us, very cool!
.github/workflows/build.yml
Outdated
| CURRENT_RELEASE=$(docker manifest inspect -v discourse/base:release | jq -r '.[0].SchemaV2Manifest.layers[] | .size / 1024 / 1024 | .*100 | round/100' | awk '{print $0; sum+= $0}; END {print sum}' | tail -n 1) | ||
| NEW_SLIM=$(docker manifest inspect -v --insecure localhost:5002/base:2.0.${{ env.TIMESTAMP }}-slim-${{ matrix.arch }} | jq -r '.SchemaV2Manifest.layers[] | .size / 1024 / 1024 | .*100 | round/100' | awk '{print $0; sum+= $0}; END {print sum}' | tail -n 1) | ||
| NEW_RELEASE=$(docker manifest inspect -v --insecure localhost:5002/base:2.0.${{ env.TIMESTAMP }}-main-${{ matrix.arch }} | jq -r '.SchemaV2Manifest.layers[] | .size / 1024 / 1024 | .*100 | round/100' | awk '{print $0; sum+= $0}; END {print sum}' | tail -n 1) | ||
| echo "current slim: ${CURRENT_SLIM} release: ${CURRENT_RELEASE}. new slim: ${NEW_SLIM} release: ${NEW_RELEASE}" |
There was a problem hiding this comment.
I think including the units will be better here.
| # Push to local repo to compare sizes | ||
| docker tag discourse/base:2.0.${{ env.TIMESTAMP }}-slim-${{ matrix.arch }} localhost:5000/base:2.0.${{ env.TIMESTAMP }}-slim-${{ matrix.arch }} | ||
| docker tag discourse/base:2.0.${{ env.TIMESTAMP }}-main-${{ matrix.arch }} localhost:5000/base:2.0.${{ env.TIMESTAMP }}-main-${{ matrix.arch }} | ||
| docker push --quiet localhost:5000/base:2.0.${{ env.TIMESTAMP }}-slim-${{ matrix.arch }} | ||
| docker push --quiet localhost:5000/base:2.0.${{ env.TIMESTAMP }}-main-${{ matrix.arch }} | ||
| # multiarch manifest is an array of schemas - [0] is amd64, [1] is arch64: Compare amd64. | ||
| CURRENT_SLIM=$(docker manifest inspect -v discourse/base:slim | jq -r '.[0].SchemaV2Manifest.layers[] | .size / 1024 / 1024 | .*100 | round/100' | awk '{print $0; sum+= $0}; END {print sum}' | tail -n 1) | ||
| CURRENT_RELEASE=$(docker manifest inspect -v discourse/base:release | jq -r '.[0].SchemaV2Manifest.layers[] | .size / 1024 / 1024 | .*100 | round/100' | awk '{print $0; sum+= $0}; END {print sum}' | tail -n 1) | ||
| NEW_SLIM=$(docker manifest inspect -v --insecure localhost:5000/base:2.0.${{ env.TIMESTAMP }}-slim-${{ matrix.arch }} | jq -r '.SchemaV2Manifest.layers[] | .size / 1024 / 1024 | .*100 | round/100' | awk '{print $0; sum+= $0}; END {print sum}' | tail -n 1) | ||
| NEW_RELEASE=$(docker manifest inspect -v --insecure localhost:5000/base:2.0.${{ env.TIMESTAMP }}-main-${{ matrix.arch }} | jq -r '.SchemaV2Manifest.layers[] | .size / 1024 / 1024 | .*100 | round/100' | awk '{print $0; sum+= $0}; END {print sum}' | tail -n 1) | ||
| echo "current slim: ${CURRENT_SLIM}MB release: ${CURRENT_RELEASE}MB. new slim: ${NEW_SLIM}MB release: ${NEW_RELEASE}MB" |
There was a problem hiding this comment.
Small non-blocking comment. If we are ever trying to split up the build, alot of the commands here can be executed in parallel.
There was a problem hiding this comment.
took a quick glance and it seems like steps can't be parallelized, so we'd have to split it up further into tasks but it'd be nice to do certainly 👍
Print out compressed image sizes for amd64 images, in PRs, adds a step that prints a line such as:
current slim: 676.45 release: 1007.99. new slim: 676.46 release: 1008.04making it easier to compare image sizes.
remove jobs from pushing on forks.
Remove jobs from scheduled jobs from running on forks