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

Add crio runtime support #57

Closed
wants to merge 9 commits into from
Closed

Add crio runtime support #57

wants to merge 9 commits into from

Conversation

jsalatiel
Copy link
Contributor

Add crio runtime support

create_crio_sysext.sh Outdated Show resolved Hide resolved
create_crio_sysext.sh Outdated Show resolved Hide resolved
create_crio_sysext.sh Outdated Show resolved Hide resolved
create_crio_sysext.sh Outdated Show resolved Hide resolved
create_crio_sysext.sh Outdated Show resolved Hide resolved
create_crio_sysext.sh Outdated Show resolved Hide resolved
@pothos
Copy link
Member

pothos commented Apr 11, 2024

Thanks, a few small suggestions but otherwise it's good

@jsalatiel
Copy link
Contributor Author

Anything else I should do here ?

exit 1
fi

export ARCH=x86-64
Copy link
Member

Choose a reason for hiding this comment

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

To properly build for the requested architecture we would have to map this value to the image name, e.g., arm64v8/alpine for arm64 (or pass --arch if we know that docker run=podman run).

EOF


docker save keepalived-build | tar --no-anchored --strip-components 1 -C "${SYSEXTNAME}" -xvf - layer.tar
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 doing this and FROM scratch one could also map a subfolder as /install_root into the container

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Afaik i can not map volumes during a container build.

My bad. This is my first PR and i didn't know changes made to my fork after the PR being created would also be in the PR. This PR should be only for the crio, not the keepalived.

Copy link
Member

Choose a reason for hiding this comment

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

Right, instead of using a Dockerfile one would need to execute the commands directly in it. I think it's also having one more advantage: With docker run --rm one would always clean up even when terminated early while with the current approach docker rmi keepalived-build would only run if the script runs without error/cancellation.

@jsalatiel jsalatiel closed this by deleting the head repository Apr 19, 2024
@rata
Copy link
Member

rata commented Apr 29, 2024

@jsalatiel did you close this PR on purpose?

@jsalatiel
Copy link
Contributor Author

It was a rookie mistake :D , but I have created another one that has already been merged

@rata
Copy link
Member

rata commented Apr 29, 2024

Awesome! Thanks and congrats! :)

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