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 multi-arch images for commits on main #7900

Merged
merged 7 commits into from
Jan 9, 2023

Conversation

ddelange
Copy link
Contributor

On commits to main, push:

  • prefecthq/prefect-dev:main-python3.X
  • prefecthq/prefect-dev:sha-424242-python3.X
  • prefecthq/prefect-dev:main-python3.X-conda
  • prefecthq/prefect-dev:sha-424242-python3.X-conda

Example

Steps 1-3 ref #7740 (comment) for multi arch docker images, towards #5388

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • This pull request includes tests or only affects documentation.
  • This pull request includes a label categorizing the change e.g. fix, feature, enhancement

@ddelange ddelange requested a review from zanieb as a code owner December 15, 2022 06:21
@netlify
Copy link

netlify bot commented Dec 15, 2022

Deploy Preview for prefect-orion ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9b29971
🔍 Latest deploy log https://app.netlify.com/sites/prefect-orion/deploys/63bc33d95957ca0009273f99
😎 Deploy Preview https://deploy-preview-7900--prefect-orion.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ddelange ddelange changed the title Build multi-arch prefecthq/prefect-dev:main-python3.X images Build multi-arch images for commits on main Dec 15, 2022
@ddelange ddelange force-pushed the docker-builds-dev-main branch from ea6f99f to c64d46a Compare December 15, 2022 11:24
@ddelange ddelange force-pushed the docker-builds-dev-main branch from c64d46a to 2db6388 Compare December 15, 2022 11:37
images: prefecthq/prefect-dev
# first tag generated here will be used in CACHE_IMAGE below
tags: |
type=raw,enable=${{ github.event_name == 'release' }},value=${{ github.ref_name }},suffix=-python${{ matrix.python-version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be skipped since this is on push/dispatch for now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing this one out, this is not intended. it should be negated, because ref_name points to a tag on release event and not a branch. The git rev-parse setup from before would return HEAD because a release event is a detached branch so is also unusable. it seems to be hard to get out which branch the release was made from.

Suggested change
type=raw,enable=${{ github.event_name == 'release' }},value=${{ github.ref_name }},suffix=-python${{ matrix.python-version }}
type=raw,enable=${{ github.event_name != 'release' }},value=${{ github.ref_name }},suffix=-python${{ matrix.python-version }}

this means there will be no cache hit on a release event, because the CACHE_IMAGE would point to the sha pattern instead of the branch pattern, and we dont have buildcache for the sha pattern. fix would be to hardcode a buildcache tag in the cache-from and cache-to based on github.matrix. so not getting it from metadata-action.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to know what branch releases are made from, we can just always pull from the cache on main?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes fine by me, will hardcode 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zanieb
Copy link
Contributor

zanieb commented Dec 15, 2022

Thanks for splitting this up! It's much easier to review.

Comment on lines +45 to +48
- name: Checkout
uses: actions/checkout@v3
with:
fetch-depth: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This reminds me of the issue with retagging images build from main on release — the image will have the wrong version information. versioneer pulls information from git into a static file on Python package build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we solved this problem with a mount directive: https://github.com/pypa/setuptools_scm#usage-from-docker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

although that does void all subsequent layer caches, so we have to do it as late as possible 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm we just copy the .git folder in during the build step which works fine. The issue is that we cannot retag these images on release, we'll need to rebuild them.

Copy link
Contributor Author

@ddelange ddelange Dec 15, 2022

Choose a reason for hiding this comment

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

ah gotcha. yes copying .git might be unwanted layer size if you have a lot of history. setuptools_scm also needs fetch-depth: 0 btw... ref pypa/setuptools-scm#480

they do however provide a 'pretend' environment variable mechanic you could combine with ${{ github.ref_name}} to 'solve' the problem

@zanieb
Copy link
Contributor

zanieb commented Dec 28, 2022

@jawnsy would you be able to take a look at this and investigate Docker credentials that can only push to prefect-dev?

@ddelange
Copy link
Contributor Author

ddelange commented Jan 4, 2023

fwiw I can see no real security concerns with using the same token for all image pushes without manual approval. the action only pushes to production on a release event (#7901), and that can only be changed if a malicious PR was merged into main

@zanieb
Copy link
Contributor

zanieb commented Jan 6, 2023

I've now added DockerHub credentials scoped to prefect-dev to the "dev" GitHub Actions Environment.

Comment on lines +14 to +15
name: Build Docker images
runs-on: ubuntu-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you'll need "environment: dev" like we do for the prod environment https://github.com/PrefectHQ/prefect/blob/main/.github/workflows/release.yaml#L186

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! d6defda

Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Sweet thanks! Let's give this a try :)

@zanieb zanieb merged commit 7c92aca into PrefectHQ:main Jan 9, 2023
@ddelange
Copy link
Contributor Author

ddelange commented Jan 9, 2023

ddelange added a commit to ddelange/prefect that referenced this pull request Jan 9, 2023
…r-builds-release

* 'main' of https://github.com/ddelange/prefect: (131 commits)
  Fix docker-builds.yaml templating syntax (PrefectHQ#8114)
  Rename Worker pools -> work pools (PrefectHQ#8107)
  Build multi-arch images for commits on main (PrefectHQ#7900)
  [Issue PrefectHQ#456] Change example of `infra_override` in docs/concepts/deployment (PrefectHQ#8101)
  Bump @playwright/test from 1.29.1 to 1.29.2 in /orion-ui (PrefectHQ#8105)
  Bump @prefecthq/orion-design from 1.1.53 to 1.1.54 in /orion-ui (PrefectHQ#8104)
  Update deployment docs to include tag and idempotency key (PrefectHQ#7771)
  Add `BaseWorker` and `ProcessWorker` (PrefectHQ#7996)
  Add Peyton and Serina as global code owners (PrefectHQ#8098)
  Add release notes for 2.7.7 (PrefectHQ#8091)
  Add youtube badge (PrefectHQ#8089)
  Adds `MAX_RRULE_LENGTH` (PrefectHQ#7762)
  Limit task run cache key size (PrefectHQ#7275)
  Add --match flag to work queues documentation (PrefectHQ#7768)
  Modify disable ssl setting tests to allow any for headers and timeout (PrefectHQ#8086)
  Add test for allow_failure and quote (PrefectHQ#8055)
  Adds `experimental_field` decorator (PrefectHQ#8066)
  add docs on migrating block documents (PrefectHQ#8085)
  Add Redoc documentation for REST API reference (PrefectHQ#7503)
  Allow disabling SSL verification (PrefectHQ#7850)
  ...
ddelange added a commit to ddelange/prefect that referenced this pull request Jan 9, 2023
…refect into docker-builds-consolidation

* 'docker-builds-release' of https://github.com/ddelange/prefect: (131 commits)
  Fix docker-builds.yaml templating syntax (PrefectHQ#8114)
  Rename Worker pools -> work pools (PrefectHQ#8107)
  Build multi-arch images for commits on main (PrefectHQ#7900)
  [Issue PrefectHQ#456] Change example of `infra_override` in docs/concepts/deployment (PrefectHQ#8101)
  Bump @playwright/test from 1.29.1 to 1.29.2 in /orion-ui (PrefectHQ#8105)
  Bump @prefecthq/orion-design from 1.1.53 to 1.1.54 in /orion-ui (PrefectHQ#8104)
  Update deployment docs to include tag and idempotency key (PrefectHQ#7771)
  Add `BaseWorker` and `ProcessWorker` (PrefectHQ#7996)
  Add Peyton and Serina as global code owners (PrefectHQ#8098)
  Add release notes for 2.7.7 (PrefectHQ#8091)
  Add youtube badge (PrefectHQ#8089)
  Adds `MAX_RRULE_LENGTH` (PrefectHQ#7762)
  Limit task run cache key size (PrefectHQ#7275)
  Add --match flag to work queues documentation (PrefectHQ#7768)
  Modify disable ssl setting tests to allow any for headers and timeout (PrefectHQ#8086)
  Add test for allow_failure and quote (PrefectHQ#8055)
  Adds `experimental_field` decorator (PrefectHQ#8066)
  add docs on migrating block documents (PrefectHQ#8085)
  Add Redoc documentation for REST API reference (PrefectHQ#7503)
  Allow disabling SSL verification (PrefectHQ#7850)
  ...
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