lxc / go-lxc

Go bindings for liblxc
https://linuxcontainers.org/lxc
Other
430 stars 76 forks source link

Fix RuntimeLiblxcVersionAtLeast() to handle ~git suffixes correctly #163

Closed eraserix closed 2 years ago

eraserix commented 2 years ago

The function RuntimeLiblxcVersionAtLeast() handles liblxc versions with a '\~' suffix incorrectly. However, Ubuntu 22.04 currently uses "5.0.0~git2209-g5a7b9ce67-0ubuntu1" as a version, which breaks the version check. This will result in incorrect results from the public function HasAPIExtension().

Fixes #162 and adds unit tests for this case.

The PR also makes the function public. This will allow LXD to use it instead of duplicating the function.

eraserix commented 2 years ago

Please can you sign-off your commit. Thanks

Fixed.

I just noticed that according to https://github.com/lxc/lxd/issues/10604, HasAPIExtension() is duplicated. Maybe it does not make sense in this case to export RuntimeLiblxcAtLeast() if it isn't required anywhere outside of this repo.

tomponline commented 2 years ago

I've updated that issue to reflect the issue at hand.

tomponline commented 2 years ago

Please can you update PR description with "Fixes #162" so your issue is closed when this is merged. Thanks

tomponline commented 2 years ago

Thanks LGTM

eraserix commented 2 years ago

I can certainly fix the go module static error. But I'm not really sure what to do about "TestExecute()" and how this failure relates to the proposed changes.

brauner commented 2 years ago

I can certainly fix the go module static error. But I'm not really sure what to do about "TestExecute()" and how this failure relates to the proposed changes.

You only need to care about the test failure you introduced obviously. :)

eraserix commented 2 years ago

You only need to care about the test failure you introduced obviously. :)

Which one would that be? Sorry, I seem to be a bit on the slow side today.

I see two failures inside the tests:

brauner commented 2 years ago

You only need to care about the test failure you introduced obviously. :)

Which one would that be? Sorry, I seem to be a bit on the slow side today.

I see two failures inside the tests:

  • TestExecute(): Seems unlikely to relate to the change, but difficult to test because it seems to require a working lxc configuration. FWIW, I get the same error message from "lxc-execute" with and without the changes, but that does not mean a thing.
  • Static analysis: This is certainly also present inside the v2-branch and seems to be triggered because an external dependency has been updated since the last successful execution of the test.

I seem to have misread the error code on my phone earlier today. Your checks all pass apparently so I think we're good to go.