osbuild / images

Image builder image definition library
Apache License 2.0
23 stars 52 forks source link

Makefile: initial version of makefile, replace GH action #954

Closed schuellerf closed 1 month ago

schuellerf commented 1 month ago

Implementing make test to be able to run the tests locally and implements make gh-action-test to run github action "unit-tests-fedora" locally

Also changes the github action to just use the implementation in Makefile

achilleas-k commented 1 month ago

A note on

and have it fail on systems that don't have dependencies.

and

TBD implement checking/installing prerequisites and running the tests without a container

Apologies if I'm making assumptions about the plan for the test Makefile target.

I get the appeal of being able to git clone the repo, run make test and have all the tests run as if by magic without needing to install dependencies and stuff. But if this is where this is going, if the plan for a future make test is to have it install required dependencies on the developer's system, I have to question the utility of it all.

First of all, there's no scenario where we don't assume some prerequisites for users. Does the container target install podman for the user as well? Are we going to assume what distro they're running or have package lists for all major distributions? Are we making it rpm only? What about Fedora vs CentOS/RHEL (no btrfs driver).

Also, any check for dependencies will either need to check that they're installed, or run a no-op script that tries to install them every time make test is called, or maybe write a file in the repo like the container info one. It seems like a lot of steps to avoid having to tell people "if you're going to compile this thing, you're going to need these packages", which is something that happens once per contributor/machine.

I don't think listing dependencies in the README and asking people who clone the repository to install some packages themselves is too much. I get that a lot of things in this repository are a little hostile to newcomers, but I doubt build dependency installation is one of them.

schuellerf commented 1 month ago

@achilleas-k you are right, although I like Makefiles that try to care about prerequisites as much as possible reducing the need to update ( / duplicate code in…) READMEs and keeping "infrastructure" or "install instructions" as code. Similar to a README where also only rpm package names are mentioned (now), this is up to future contributors to just add the section/implementation for other distros.

But sure, this can get out of hands quickly. I'll just update the README.md now (as it's out of date already a while ago) and finish this PR

achilleas-k commented 1 month ago

But sure, this can get out of hands quickly. I'll just update the README.md now (as it's out of date already a while ago) and finish this PR

What's outdated in the README? The "out of date" link opens an old commit for tests.yaml.

Which brings me to #959 and something I realised while writing that. The github action is actually outdated. The krb5-devel package isn't needed for unit tests. It's needed by the koji uploader, but we don't test that (which is the actual problem, I guess).

The issue of "where should uploaders live" (images or osbuild-composer) is a long-standing question we haven't entirely figured out yet, so I wont go into it here. But as it stands, right now, krb5-devel isn't a build nor a test dependency of osbuild/images. It's a build dependency of any project that imports pkg/upload/koji.

thozza commented 1 month ago

Besides for the action not working, this PR now looks good to me. :+1:

schuellerf commented 1 month ago

Should we use the make gh-action-test target for the CentOS Stream tests as well?

I'll check the implications and do a followup-PR

schuellerf commented 1 month ago

… rebased to main, lost approvals … :-/