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.

Sign up for GitHub

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

Jump to bottom

1898 byoi #1912

Merged
merged 14 commits into from
Oct 24, 2023
Merged

1898 byoi #1912

merged 14 commits into from
Oct 24, 2023

Conversation

jimmykarily
Copy link
Contributor

@jimmykarily jimmykarily commented Oct 11, 2023 •
edited
Loading

Fixes #1898

@jimmykarily jimmykarily force-pushed the 1898-byoi branch 7 times, most recently from a307a55 to c1d4dbf Compare October 19, 2023 06:45
@jimmykarily jimmykarily force-pushed the 1898-byoi branch 6 times, most recently from 2b73df5 to 5afe742 Compare October 23, 2023 17:47
@jimmykarily jimmykarily mentioned this pull request Oct 24, 2023
Closed
5 tasks
jimmykarily
jimmykarily commented Oct 24, 2023
View reviewed changes
images/Dockerfile.ubuntu Outdated Show resolved Hide resolved
mauromorales and others added 4 commits October 24, 2023 09:51
@mauromorales @jimmykarily
Refactor flavors and dockerfiles
0bcf528 
Fixes #1898

Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Use FRAMEWORK_FLAVOR variable to avoid confusion with FLAVOR
e757edd 
Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Fix pipeline by adding mandatory value
9fd07df 
Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Fix ubuntu Dockerfile
503e150 
Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily jimmykarily marked this pull request as ready for review October 24, 2023 06:52
@jimmykarily
Apply suggestions from code review
6f9c9e1 
Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Remove TODO (already implemented) and non-used ARG "IMAGE"
5d5e4e1 
Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Remove TODOs
2b9c943 
moved here: #1897 (comment)

Signed-off-by: Dimitris Karakasilis <[email protected]>
mauromorales
mauromorales approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@mauromorales mauromorales left a comment

Choose a reason for hiding this comment

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

lgtm 👏

@Itxaka Itxaka requested a review from a team October 24, 2023 08:17
Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
Earthfile Show resolved Hide resolved
Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
Earthfile Show resolved Hide resolved
Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
Earthfile Show resolved Hide resolved
Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
Earthfile Show resolved Hide resolved
Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
Earthfile Show resolved Hide resolved
Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
Earthfile
SAVE ARTIFACT --keep-own /. rootfs

uki-artifacts:
ARG --required FAMILY # The dockerfile to use
Copy link
Member

Choose a reason for hiding this comment

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

damn, cant we make this a global thing instead of having them in each target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not. Anything we make global becomes accessible to every target, even those that don't need it. Then you can't make it --required either (since it's not required by all targets, right?). It looks ugly right now but I'm hoping on the next iteration, the only thing most targets will need is the a "base image" arg which will be a pre-built image. That one will be build directly with docker to ensure that we can always build with just docker, no earthly required.

Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
images/Dockerfile.ubuntu
FROM base-ubuntu-22-lts AS arm64-ubuntu
FROM base-ubuntu-22-lts AS arm64-ubuntu-22-lts
FROM base-ubuntu-20-lts AS arm64-ubuntu-20-lts
FROM base-ubuntu-22.04 AS arm64-ubuntu-23.04
Copy link
Member

Choose a reason for hiding this comment

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

again from 22 to 23?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mauromorales did I mess up on rebase?

Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
images/Dockerfile.ubuntu Outdated Show resolved Hide resolved
Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
Earthfile
@@ -604,10 +644,21 @@ iso:


iso-uki:
FROM ubuntu
Copy link
Member

Choose a reason for hiding this comment

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

why ubuntu? A smaller image like alpine is better suited for this, especially if its just to run a script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alpine had no bash:

~/workspace/kairos/kairos (1898-byoi)*$ docker run -it --rm alpine /bin/bash
docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "/bin/bash": stat /bin/bash: no such file or directory: unknown.

We could implement the script in pure sh but we opted for an image that has bash.

Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
naming.sh Show resolved Hide resolved
Itxaka
Itxaka reviewed Oct 24, 2023
View reviewed changes
Earthfile Show resolved Hide resolved
Itxaka
Itxaka requested changes Oct 24, 2023
View reviewed changes
Copy link
Member

@Itxaka Itxaka left a comment

Choose a reason for hiding this comment

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

some changes in regard to framework target and dockerfiles

@jimmykarily
PR comments
20252ee 
Signed-off-by: Dimitris Karakasilis <[email protected]>
Itxaka
Itxaka approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@Itxaka Itxaka left a comment

Choose a reason for hiding this comment

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

looks good to me!

@jimmykarily
Remove naming.sh script from base image
9126244 
Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Make sure we use the "fixed" kairos version with the remove "+" symbols
d0f3efd 
when creating a tag for container images.

Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Setup qemu and docker buildx in reusable-build-flavor workflow
a587567 
because otherwise it fails to build rpi images

Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Remove not needed env variables
c6ba2dc 
Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Remove temporary change
4fb17dc 
Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily jimmykarily merged commit 664c96f into master Oct 24, 2023
10 of 11 checks passed
@jimmykarily jimmykarily deleted the 1898-byoi branch October 24, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Itxaka Itxaka Itxaka approved these changes

@mauromorales mauromorales mauromorales approved these changes

Assignees
No one assigned
Labels
None yet
Projects
🧙Issue tracking board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dockefiles should separate the base image from the "build strategy" (aka flavor)
3 participants
@jimmykarily @mauromorales @Itxaka