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

Makefile: add cross-tar to cross-packages #953

Merged
merged 1 commit into from Dec 13, 2022

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Dec 12, 2022

Also build the "distroless" binary tarball with the corss-packages
target similar to the deb and rpm packages.

Also, change packaging-tests target to depend on cross-packages so that
it also tests the binary tarball. Even if it currently only tests the
buildability of the tarball as there is no actual e2e test to install
and verify it. Adding a packaging e2e-test for the binary tarball is
left as a future exercise.

@marquiz
Copy link
Contributor Author

marquiz commented Dec 12, 2022

ping @klihub WDYT?

The binary tarball hasn't traditionally been considered as "packaging" in cri-rm but it'd be nice to have "one make target to rule them all"

Actually, maybe we should add cross-tar as a dependency to cross-packages as well to make it symmetric 🤔

klihub
klihub previously approved these changes Dec 12, 2022
Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

This is fine, but is it enough ? AFAICT, with these changes we only build the tarball as a 'packaging test'. IIRC, the other packaging tests check that cri-resmgr can be installed from the built packages, it can start up, and that a container can be created once it is running.

Shouldn't we do the same for the tar package ? It's a bit more involved than a few one-liners, because we'd need to teach test/e2e/run.sh, demo/lib/vm.bash et al. to handle something like binsrc=tarball. So I'm also fine with doing that in another PR... and maybe not for this release.

@marquiz
Copy link
Contributor Author

marquiz commented Dec 12, 2022

packaging tests check that cri-resmgr can be installed from the built packages, it can start up, and that a container can be created once it is running.

Shouldn't we do the same for the tar package ? It's a bit more involved than a few one-liners, because we'd need to teach test/e2e/run.sh, demo/lib/vm.bash et al. to handle something like binsrc=tarball. So I'm also fine with doing that in another PR... and maybe not for this release.

Yeah, that's what I commented in the commit message, too. There's no real packaging e2e-test for it yet. But I think we can address that gap later. I think testing even buildability is better than nothing...

@marquiz marquiz changed the title Makefile: add cross-tar to packaging-tests Makefile: add cross-tar to cross-packages Dec 12, 2022
@marquiz
Copy link
Contributor Author

marquiz commented Dec 12, 2022

I changed to scope of the PR. Title and message adjusted accordingly

@klihub
Copy link
Contributor

klihub commented Dec 13, 2022

packaging tests check that cri-resmgr can be installed from the built packages, it can start up, and that a container can be created once it is running.
Shouldn't we do the same for the tar package ? It's a bit more involved than a few one-liners, because we'd need to teach test/e2e/run.sh, demo/lib/vm.bash et al. to handle something like binsrc=tarball. So I'm also fine with doing that in another PR... and maybe not for this release.

Yeah, that's what I commented in the commit message, too. There's no real packaging e2e-test for it yet. But I think we can address that gap later. I think testing even buildability is better than nothing...

Definitely. We can go with this and add binsrc=tarball support to enable install+start+create-pod test in a further PR.

Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

There is a typo (extra '§' I assume) in the commit message: 'Also build §the "distroless"', which you might want to fix. Otherwise good to go in.

Also build the "distroless" binary tarball with the corss-packages
target similar to the deb and rpm packages.

Also, change packaging-tests target to depend on cross-packages so that
it also tests the binary tarball. Even if it currently only tests the
buildability of the tarball as there is no actual e2e test to install
and verify it. Adding a packaging e2e-test for the binary tarball is
left as a future exercise.
@marquiz
Copy link
Contributor Author

marquiz commented Dec 13, 2022

There is a typo (extra '§' I assume) in the commit message: 'Also build §the "distroless"', which you might want to fix.

Woops, late night fat-fingering. Thanks for spotting this. Fixed now

@marquiz
Copy link
Contributor Author

marquiz commented Dec 13, 2022

ping @klihub @askervin

Copy link
Contributor

@askervin askervin left a comment

Choose a reason for hiding this comment

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

LGTM

@klihub klihub merged commit abcdabd into intel:master Dec 13, 2022
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.

None yet

4 participants