Test Information

[m-ildefons]

Annotate the test information with Rancher/RKE version information. This helps the test report HTML files to be less ambiguous about the configuration under test.

[albinsun]

Rancher part LGTM. Have some comments on Harvester part.

[albinsun]

It's better to keep current implementation in try and new one in exception.

try:
    self._version = parse_version('8.8.8' if 'master' in ver else ver)
except:
    #YOUR_NEW_IMPLEMENTATION

[m-ildefons]

No, why?

[m-ildefons]

The goal of this PR is to enhance the metadata annotations of the test results and go from this:

to this:

[lanfon72]

Although Rancher is not using master-head, we would still need to consider cases like:

1. v2.9.1-rc6
2. v2.9-2f9a7509687f559d772be75d61df7f7409803883-head

As the second case would be the latest of v2.9, we need to fill the patch version for it. (to notice that there will not have something like v2.9.1-sha-head)

so, I would suggest that we could update it to something like

tests/apiclient/harvester_api/api.py

Lines 43 to 55 in 4d4d22b

	try:
	# XXX: https://github.com/harvester/harvester/issues/3137
	# Fixed as:
	# 1. va.b-xxx-head => va.b.99
	# 2. master-xxx-head => v8.8.99
	cur, _, _ = ver.split('-')
	cur = "v8.8" if "master" in cur else cur
	self._version = parse_version(f"{cur}.99")
	except ValueError:
	self._version = parse_version(ver)
	# store the raw version returns from `server-version` for reference
	self._version.raw = ver
	return self._version

which we already implemented in HarvesterAPI.
To notice that Python usually use try-except to control the flow.¹

Footnotes

1. https://devblogs.microsoft.com/python/idiomatic-python-eafp-versus-lbyl/ ↩

[m-ildefons]

That was what I did initially, but @albinsun wasn't happy with it.

[albinsun]

I was talking about the order but not drop error handling.
My thought was

1. Existing implementation works for most cases
   (Except head form like v2.9-2f9a7509687f559d-head)
2. I thought you are dealing the head form case, so it's natually to suggest

   try:
      # Existing implementation
   except:
      # Dealing exceptions
   

Anyway, per discussed, It's reasonable and OK for me to align with Harvester API side implementation, thx.

[m-ildefons]

I think I have a good implementation now. It tries hard to parse the version, it keeps the suffix information for later reference and if all fails, it behaves like the Harvester API implementation.
I noticed that with my version of the pkg_resources library parse_version never raises an exception, but will produce a LegacyVersion object in case it doesn't know how to deal with it:

Python 3.6.15 (default, Sep 23 2021, 15:41:43) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from pkg_resources import parse_version
>>> parse_version("foobar-noversion")
<LegacyVersion('foobar-noversion')>

Therefore I'm quite sure that the exception case will not raise another error.

[albinsun]

As I know our daily env. adopts python 3.10 which has different behavior on parse_version

Python 3.10.14 (main, Mar 24 2024, 08:01:04) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from pkg_resources import parse_version
<stdin>:1: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
>>> parse_version("foobar-noversion")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/albin/Workspace/albinsun/tests/.tox/py310/lib/python3.10/site-packages/pkg_resources/_vendor/packaging/version.py", line 200, in __init__
    raise InvalidVersion(f"Invalid version: '{version}'")
pkg_resources._vendor.packaging.version.InvalidVersion: Invalid version: 'foobar-noversion'

[lanfon72]

LegacyVersion had been dropped in pypa/packaging#407

[lanfon72]

To notice that the suffix is meaningful for the Version instance to compare with others, for example:

so we should only handle the case like v2.9-2f9a7509687f559d772be75d61df7f7409803883-head, as I know, rancher will not use -dev-<date> format, so it is fine to not consider the case.

[albinsun]

LGTM

[m-ildefons]

I've reduced this PR to the bare essential, which is to have the raw version string in the test metadata.