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

refactor: Reorganize Build Process #1867

Merged
merged 56 commits into from
Nov 5, 2024
Merged

refactor: Reorganize Build Process #1867

merged 56 commits into from
Nov 5, 2024

Conversation

m2Giles
Copy link
Member

@m2Giles m2Giles commented Nov 1, 2024

This significantly reworks the build system for Bluefin/Aurora.

Changes:
Consolidates Workflows
Removes Surface and Asus images in favor of a single HWE image
Rewrite of local dev Justfile
Reorganization and Consolidation of build files

Add new files

Closes: #1841

Signed-off-by: m2 <69128853+m2Giles@users.noreply.github.com>

This significantly reworks the build system for Bluefin/Aurora.

Changes:
Consolidates Workflows
Removes Surface and Asus images in favor of a single HWE image
Refwrite of local dev Justfile
Reorganization and Consolidation of build files

Add new files

Signed-off-by: m2 <69128853+m2Giles@users.noreply.github.com>
@bsherman bsherman self-requested a review November 1, 2024 17:22
Keep the asus/surface images around until a service unit is written to rebase people onto hwe name images.
Signed-off-by: m2 <69128853+m2Giles@users.noreply.github.com>
Signed-off-by: m2 <69128853+m2Giles@users.noreply.github.com>
@m2Giles
Copy link
Member Author

m2Giles commented Nov 1, 2024

#1841
#1839

@m2Giles
Copy link
Member Author

m2Giles commented Nov 1, 2024

I see how to pass strings that turn into json objects now.

I think we will still want a matrix in the reusable build but it just takes inputs.

@castrojo
Copy link
Member

castrojo commented Nov 1, 2024

@befanyt got any opinions here? If we're going to build a new world we might as well get new eyes. 😄

@m2Giles m2Giles requested a review from p5 November 3, 2024 15:41
m2Giles and others added 4 commits November 3, 2024 10:47
This cleans up whitespace and EOL to be consistent.

Only ran across .github, build_files, just and root dir files.

This refactor seems like a good time to introduce it, even if it's not
always running, it's a standard.
Copy link
Contributor

@bsherman bsherman left a comment

Choose a reason for hiding this comment

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

Looking this over... this is a hugely valuable effort.

However, I think we should go one step further. We should be using the value of the just recipes in our github action workflow.

At the least, the just build recipe should be used in reusable-build.yml to build the images.
All the tagging logic probably makes sense to remain in the workflow since that's external to the container build itself. Similarly, the push operation doesn't need to be in just, and it ties closely to the tags.

We could possibly use just build-rechunk in the workflow as well... Looking for feedback from @m2Giles on that.

But the primary value from doing this is ability to build container images the same way both local and in github actions. That's huge!

Fixing a bug in the image build can be validated before pushing to a PR even.

@m2Giles
Copy link
Member Author

m2Giles commented Nov 3, 2024

I think the only thing needed to change to make that a reality is to make sure build container and rechunk can run completely as root.

The only major concern is not having enough space on builds when copying to the rootful podman store.

@bsherman
Copy link
Contributor

bsherman commented Nov 3, 2024

I think the only thing needed to change to make that a reality is to make sure build container and rechunk can run completely as root.

Ah, like an additional flag to the build recipe to run it as root? Yes, then it would match workflow behavior.

The only major concern is not having enough space on builds when copying to the rootful podman store.

This should be addressed by running build as root, no?

@m2Giles
Copy link
Member Author

m2Giles commented Nov 3, 2024

I would run the just command with sudo.

I've already put in a bunch of sudoif's so this should be just testing to make sure it works as intended.

I may add the secureboot check to the justfile.

This avoids confusion where we actually do use "fedora_version" to
reference Fedora version numbers (40, 41, etc), but for the input and
matrix use cases we actually intend to refer to the Bluefin streams:
gts, stable, latest, beta
@bsherman
Copy link
Contributor

bsherman commented Nov 4, 2024

After further review and discussion, while we DO want to make use of the new and improved just recipes within the github workflows, that effort can be iterated on. As is, this is already large and holding up some needed fixes. So we should make an issue to reduce the workflow specific code by calling just instead.

I'm continuing to review the existing changes tonight. So far looking nice.

bsherman
bsherman previously approved these changes Nov 5, 2024
Copy link
Contributor

@bsherman bsherman left a comment

Choose a reason for hiding this comment

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

I have spent a bunch of time testing the justfile locally, looking through the build script changes (made a few changes there myself to fix numbering of build scripts), looking through the workflow yaml (fixed some naming conventions), and looking at the GREEN successful builds validating the podman build args.

I think this is looking good.

A great improvement and we'll keep on making it better.

Thanks @m2Giles !

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 5, 2024
@bsherman bsherman enabled auto-merge November 5, 2024 03:48
@bsherman bsherman added this pull request to the merge queue Nov 5, 2024
Merged via the queue into main with commit f833e1f Nov 5, 2024
54 checks passed
@bsherman bsherman deleted the refactor-build branch November 5, 2024 04:47
tulilirockz pushed a commit to tulilirockz/bluefin that referenced this pull request Nov 5, 2024
Signed-off-by: m2 <69128853+m2Giles@users.noreply.github.com>
Co-authored-by: Benjamin Sherman <benjamin@holyarmy.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discussion] Refactor Image Build
5 participants