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

Enable image builds with badge and add os-ref param for manual builds #493

Merged
merged 3 commits into from
Apr 29, 2024

Conversation

dbast
Copy link
Contributor

@dbast dbast commented Sep 21, 2023

This updates the existing build action to:

  • add an os-ref input parameter to be able to do manual workflow_dispatch builds where any seedsigner branch can be combined with any seedsigner-os ref (tag, branch, sha1). (default = the default branch of both repos)
  • produce unique (git describe based)image file names that have the seedsigner and seedsigner-os version encoded, e.g. seedsigner_os.os0.6.0-61-g9fafebe_sw0.7.0-40-g4b7c895.pi0.img to know exactly what version an image is based on.
  • simplify the cache action step: restore + save is automatically done by the plain action and its included post-action step. Some testing shows that the cache action reduces the build time to 50% (~30mins).
  • enable daily builds = daily PoW that everything works, sources exist, and builds fine together. And it also enables easily testing of merged PRs of the last days for everybody without locally building anything or waiting for a manual workflow_dispatch run. (this also keeps the cache warm, so succeeding triggered builds are a lot faster). one build per merge
  • lower the retention-days to 7 (default 90 days), to not consume to much artifact space with the daily builds enabled. newer builds overwrite older builds in case the artifact cache is full

For more details see inline comments.

Succeeding PRs can also enable builds for every merge to the dev/main branch or for triggering builds when a tag is added.

some links:

Description

Describe the change simply. Provide a reason for the change.

Include screenshots of any new or modified screens (or at least explain why they were omitted)

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

  • I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

  • No, I’m a fool
  • Yes
  • N/A

I have tested this PR on the following platforms/os:

Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.

@dbast dbast force-pushed the daily-builds branch 2 times, most recently from ea81a33 to bb7e4ee Compare September 22, 2023 10:04
@kdmukai
Copy link
Contributor

kdmukai commented Mar 3, 2024

Out of my area of expertise, but this sounds awesome. Opens the door for having less technical people try out PRs or the latest merged into dev, ya?

A few thoughts @dbast:

  • Is the daily build necessary? We have very infrequent merges so it makes more sense to me to just have:
    • A build for each PR
    • A build for each merge
  • Expiration: I think it would be more useful to have the resulting images stick around for longer.
    • For PRs: Updated/replaced on PR updates, remains available until the PR is merged or closed (is that possible?)
    • For merges: Keep the last n merges?

This updates the existing build action to:
* add an os-ref input parameter to be able to do manual workflow_dispatch
  builds where any seedsigner branch can be combined with any seedsigner-os
  ref (tag, branch, sha1). (default = the default branch of both repos)
* produce unique (git describe based)image file names that have the
  seedsigner and seedsigner-os version encoded, e.g.
  seedsigner_os.os0.6.0-61-g9fafebe_sw0.7.0-40-g4b7c895.pi0.img
  to know exactly what version an image is based on.
* simplify the cache action step: restore + save is automatically done
  by the plain action and its included post-action step. Some testing
  shows that the cache action reduces the build time to 50% (~30mins).
* enable daily builds = daily PoW that everything works, sources exist,
  and builds fine together. And it also enables easily testing of merged
  PRs of the last days for everybody without locally building anything or
  waiting for a manual workflow_dispatch run. (this also keeps the cache
  warm, so succeeding triggered builds are a lot faster).
* lower the retention-days to 7 (default 90 days), to not consume to much
  artifact space with the daily builds enabled.

For more details see inline comments.

Succeeding PRs can also enable builds for every merge to the dev/main
branch or for triggering builds when a tag is added.

# Conflicts:
#	README.md
@dbast
Copy link
Contributor Author

dbast commented Apr 12, 2024

@kdmukai Rebased the PR and addressed your review findings: one build per merged PR, PR image builds only on demand as resource hungry (can be triggered via comment change, see code), retention days increased to maximum possible.

@dbast dbast changed the title Enable daily builds with badge and add os-ref param for manual builds Enable image builds with badge and add os-ref param for manual builds Apr 12, 2024
@newtonick
Copy link
Collaborator

ACK

@newtonick newtonick merged commit e5d3133 into SeedSigner:dev Apr 29, 2024
6 checks passed
@dbast dbast deleted the daily-builds branch May 4, 2024 08:33
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.

3 participants