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

12648 epic upgrade to a newer debian 12 ci #9036

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

rahulinux
Copy link
Contributor

Problem

Summary of changes

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

cache-from: type=registry,ref=cache.neon.build/build-tools:cache-${{ matrix.arch }}
cache-to: ${{ github.ref_name == 'main' && format('type=registry,ref=cache.neon.build/build-tools:cache-{0},mode=max', matrix.arch) || '' }}
tags: neondatabase/build-tools:${{ inputs.image-tag }}-${{ matrix.arch }}
file: docker/debian/${{ matrix.os }}/Dockerfile.build-tools
Copy link
Member

Choose a reason for hiding this comment

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

Instead of copying files, can we pass the base image (debian:bookworm-slim/debian:bullseye-slim) as an argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's possible, however there are some package changes for each base image for that we need to add conditional checks to install package based on code-name, for that reason I created copy.

Copy link
Member

Choose a reason for hiding this comment

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

We have

ARG DEBIAN_VERSION_CODENAME=bullseye

So by setting it to the correct version it might work


merge-images:
needs: [ build-image ]
runs-on: ubuntu-22.04
strategy:
matrix:
os: [ bullseye, bookworm ]
Copy link
Member

Choose a reason for hiding this comment

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

GitHub already has runner.os, using matrix.os is confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

runner and matrix both are different thing, we can use code-name which they call in Debian term or we can call it debians

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update this with debian_version

@@ -99,5 +103,5 @@ jobs:
- name: Create multi-arch image
run: |
docker buildx imagetools create -t neondatabase/build-tools:${IMAGE_TAG} \
Copy link
Member

Choose a reason for hiding this comment

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

The resulting tag should also include the debian version

Comment on lines 106 to 107
neondatabase/build-tools:${IMAGE_TAG}-${{ matrix.os }}-x64 \
neondatabase/build-tools:${IMAGE_TAG}-${{ matrix.os }}-arm64
Copy link
Member

Choose a reason for hiding this comment

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

It seems arch and os are swapped here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one is confusing, should we have matrix for this CI stage as well?

docker-compose/compute_wrapper/Dockerfile Show resolved Hide resolved
@rahulinux rahulinux force-pushed the 12648-epic-upgrade-to-a-newer-debian-12-ci branch 2 times, most recently from 59411f8 to 33538f6 Compare September 18, 2024 17:12
@rahulinux rahulinux force-pushed the 12648-epic-upgrade-to-a-newer-debian-12-ci branch from 33538f6 to 6e3b687 Compare September 19, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants