search

seccomp / libseccomp-golang

The libseccomp golang bindings repository
BSD 2-Clause "Simplified" License
268 stars 56 forks source link

CI/GHA: test against all minor supported libseccomp versions #70

Closed kolyshkin closed 3 years ago

kolyshkin commented 3 years ago

In short:

Addresses: https://github.com/seccomp/libseccomp-golang/pull/65#issuecomment-917166866

Please review commit by commit.

drakenclimber commented 3 years ago

Thanks @kolyshkin. A matrix seemed like a good way to meet everyone's testing needs; I'm glad you added it.

Reviewed-by: Tom Hromatka <tom.hromatka@oracle.com>

kolyshkin commented 3 years ago

Looking more into libseccomp-golang, it seems that it will benefit from testing against older supported libseccomp releases, not just the latest. Currently the minimum supported version is 2.2.0, let's try to add it.

kolyshkin commented 3 years ago

Nice -- added libseccomp 2.3.x and 2.4.x revealed a bug in TestNotif -- it checks the API level but do not check libseccomp version :)

Will fix later today, changing to DRAFT for now.

kolyshkin commented 3 years ago

Looks like this is finally ready; took me a while to fix all the issues found 😅

kolyshkin commented 3 years ago

Ideally, I think, it makes sense to cache the result of libseccomp compile (and avoid git clone/configure/make install if the cache is sufficient), but this can definitely be implemented separately on top of this.

kolyshkin commented 3 years ago

@drakenclimber drakenclimber added the enhancement label 5 days ago

With all the fixes added to support older libseccomp version, I'd characterize this as bugfix rather than enhancement now.

kolyshkin commented 3 years ago

@drakenclimber @pcmoore PTAL.

If sending patches to the ML is preferable, please let me know (currently, CONTRIBUTING.md says otherwise).

drakenclimber commented 3 years ago

If sending patches to the ML is preferable, please let me know (currently, CONTRIBUTING.md says otherwise).

I personally prefer github pull requests. But I wear many hats and unfortunately libseccomp-golang isn't one of them this week. The Linux Plumbers Conference is currently going on all week.

pcmoore commented 3 years ago

If sending patches to the ML is preferable, please let me know (currently, CONTRIBUTING.md says otherwise).

I personally prefer github pull requests. But I wear many hats and unfortunately libseccomp-golang isn't one of them this week. The Linux Plumbers Conference is currently going on all week.

+1 to what @drakenclimber already said. This has been a particularly difficult week on my end due to some personal issues, LPC, and a handful of high-priority and high-visibility SELinux kernel issues flaring up. I'm working my way through the libseccomp-golang PRs right now ...

pcmoore commented 3 years ago

Thanks again for the patches @kolyshkin, but I have a few general comments, in no particular order:

My apologies if all of the above seem a bit trivial and nit-picky, but as you are starting to contribute more - thank you! - you're going to be subjected to a bit more "tough love" if you will :) Please leave this PR as-is, I'll fix these things up when I hear back from @drakenclimber (below), but it is something to keep in mind for the future.

@drakenclimber, this PR has changed quite a bit since you last looked at it, does your Reviewed-by tag still stand?

kolyshkin commented 3 years ago

I dislike combining unrelated patches into a single PR simply because while working on item A you happened to stumble across item B. I get it, it's extremely tempting to just do everything in one PR, but it is a bit of a mess to review. I'd much rather you see things where you post one PR, get it merged, and then post the other; or post both PRs at the same time with a comment that PR A is dependent on PR b (or something similar).

If you got an impression that I piled up everything I can change into this single PR, let me assure you this is not so. It all started with "let's add libseccomp versions that this package seems to support to CI", and the rest was added to make the CI green.

The only non-really-related stuff is "seccomp_internal.go: simplify version checks" and "test: execInSubprocess: simplify", and I will move it out of this commit.

Be careful when you talk about "supported versions" in the commit descriptions and code comments

Will reword.

My apologies if all of the above seem a bit trivial and nit-picky

Definitely no need to apologize, any constructive feedback (and "tough love") is very welcome and truly appreciated.

Please leave this PR as-is.

No problem, I'd rather fix it in place.

kolyshkin commented 3 years ago

Rebased on top of the current main

kolyshkin commented 3 years ago

@drakenclimber @pcmoore PTAL 🙏🏻

pcmoore commented 3 years ago

Patience please. This week, and last week for that matter, have been busy with conferences as well as the usual stuff. On a personal note, the late summer, early fall weather has been really nice where I live too so I've been trying to enjoy that outside of work.

We see the activity on this, and appreciate it, but please be patient and allow us the time to review it.

drakenclimber commented 3 years ago

Thanks, @kolyshkin. The most recent changes look good to me.

Acked-by: Tom Hromatka <tom.hromatka@oracle.com>

pcmoore commented 3 years ago

Merged at HEAD 6a935f5222db4e0543c15142f4ae9d3830d1d671, thanks @kolyshkin!