[apiclient] restructure models and managers #982

lanfon72 · 2023-11-09T16:36:53Z

Changes

UPDATE restructure managers.py
- add folder managers
- add __init__.py for the folder as a module
- add base.py for BaseManager and constants
- add files for related managers (hosts, images, volumes, keypairs, settings, networks, templates, supportbundles, storageclasses, clusternetworks, volumesnapshots, virtualmachines), backups for BackupManager and VirtualMachineSnapshotManager, internal and longhorn for those operations not available in harvester dashboard. (managers would be imported in __init__.py)
UPDATE restructure models.py
- add folder models
- add __init__.py for the folder as a module
- add base.py for constants
- add files for related Specs (backups, settings, virtualmachines, volumes)
NEW type stub files
- add .pyi files into managers folder for related managers
- add .pyi files into models folder for related specs
- add api.pyi for api.py
- add py.typed to mark the lib having type information¹
NEW unit tests
- add test_load_managers to test api.load_managers
- add test_for_version to test manager.for_version

The restructure is necessary that we are going to support multiple cluster version in the API with new introduced method api.load_managers, for implement new api operations, we can simply inherit old managers to overwrite old version. (you can check the tests/test_managers.py::test_for_version for more details.

PEP561 https://peps.python.org/pep-0561/ ↩

albinsun · 2023-11-14T06:31:32Z

Do we have test runs/reports on restructured suites?

lanfon72 · 2023-11-14T07:04:41Z

Do we have test runs/reports on restructured suites?

This change will not affect to existing test cases, so I didn't trigger any CI build for it.
The change might affect to existing import related to apiclient, and I had confirmed that this will compatible with old one by executing pytest --collect-only harvester_e2e_tests/

albinsun

Test will fail when import typing.Final, which is new in python 3.8 and not compatible to our current env.
Ref. harvester-runtests#410

...
'/var/harvester_test_410_harvester-runtests/.tox/py36/lib64/python3.6/site-packages/yaml/__init__.py'>
apiclient/harvester_api/models/base.py:1: in <module>
    from typing import Final
E   ImportError: cannot import name 'Final'
...

lanfon72 · 2023-11-15T16:19:27Z

As we are leaving python 3.6 (#970), I will add a temporarily fix to let it compatible with 3.6, then we can revert the commit when we change to use 3.10+

(Executed on harvester-runtests#412, and #411 is the run before the fix)

albinsun · 2023-11-18T05:51:00Z

As we are leaving python 3.6 (#970), I will add a temporarily fix to let it compatible with 3.6, then we can revert the commit when we change to use 3.10+

(Executed on harvester-runtests#412, and #411 is the run before the fix)

Hi, could you help to confirm #412 test result?
They both error with UserWarning: conflicting basepython version (set 3.6, should be 3.8) for env 'py38';....

And the 412 report looks empty.

411

TASK [tests : Run the functional tests] ****************************************
Wednesday 15 November 2023  15:58:31 +0000 (0:00:00.057)       0:01:18.644 **** 
fatal: [pcloud003]: FAILED! => changed=true 
  cmd: tox -e py36 --result-json=test_result.json -- -vvl `cat run_filters/all.txt`  --json-report --html=test_report/test_result.html -m 'not delete_host and not upgrade' -k 'apis2' > test_result.txt
  delta: '0:00:32.544962'
  end: '2023-11-15 08:23:59.325522'
  msg: non-zero return code
  rc: 1
  start: '2023-11-15 08:23:26.780560'
  stderr: |-
    /usr/lib/python3.6/site-packages/tox/config/__init__.py:672: UserWarning: conflicting basepython version (set 3.6, should be 3.8) for env 'py38';resolve conflict or set ignore_basepython_conflict
      testenv_config.envname,
  stderr_lines: <omitted>
  stdout: ''
  stdout_lines: <omitted>
...ignoring

412

TASK [tests : debug] ***********************************************************
Wednesday 15 November 2023  16:22:33 +0000 (0:00:00.046)       0:01:08.614 **** 
ok: [pcloud003] => 
  msg: tox -e py36 --result-json=test_result.json -- -vvl `cat run_filters/all.txt`  --json-report --html=test_report/test_result.html -m 'not delete_host and not upgrade' -k 'apis2'

TASK [tests : Run the functional tests] ****************************************
Wednesday 15 November 2023  16:22:33 +0000 (0:00:00.068)       0:01:08.683 **** 
fatal: [pcloud003]: FAILED! => changed=true 
  cmd: tox -e py36 --result-json=test_result.json -- -vvl `cat run_filters/all.txt`  --json-report --html=test_report/test_result.html -m 'not delete_host and not upgrade' -k 'apis2' > test_result.txt
  delta: '0:00:27.348655'
  end: '2023-11-15 08:47:55.727753'
  msg: non-zero return code
  rc: 1
  start: '2023-11-15 08:47:28.379098'
  stderr: |-
    /usr/lib/python3.6/site-packages/tox/config/__init__.py:672: UserWarning: conflicting basepython version (set 3.6, should be 3.8) for env 'py38';resolve conflict or set ignore_basepython_conflict
      testenv_config.envname,
  stderr_lines: <omitted>
  stdout: ''
  stdout_lines: <omitted>
...ignoring

lanfon72 · 2023-11-18T06:29:29Z

412 will not have test results is expected, it is targeting to execute test cases not existing (then it would be same as —collect-only)
Base version conflicts is expected which is existing issue but will not affect to test execution

albinsun

As comments, please help to add comment on key properties and some more unittests to increase readability, it will be helpful.

albinsun · 2023-11-22T09:59:25Z

apiclient/tests/test_managers.py

+        self.assertEqual(B0.for_version('1.2.0'), B120)
+        self.assertEqual(B0.for_version('1.1.3'), B113)
+        self.assertEqual(B0.for_version('1.3.0'), B130)
+        self.assertEqual(B0.for_version('1.1.4'), B114)


Better to have multi-manager inheritance case which are more close to real usage in code.

for example

>>> from testo import BaseManager >>> class AManager(BaseManager): ... pass ... >>> class AManager_110(AManager): ... support_to = '1.1.0' ... >>> class BManager(BaseManager): ... pass ... >>> class BManager_110(BManager): ... support_to = '1.1.0' ... >>> BManager.for_version('1.1.0') == BManager_110 True >>> BManager.for_version('1.2.0') == BManager_110 True >>> BManager.for_version('1.0.0') != BManager_110 True >>> BManager.for_version('1.1.0') != AManager_110 True >>> AManager.for_version('1.1.0') == AManager_110 True ...

The unit test is focusing on test class method for_version, it is designed to seeking the most fit sub-class.

For multiple managers inherited from the BaseManager, is the flexibility of the class method, so it would not be included in unit test. (out of scope)

It is very general that we will not test inherited class behavior, because it's the contract of inheritance, derived class should follow parents signature and it's parents methods should always able to be used in derived.

apiclient/harvester_api/managers/base.py

lanfon72 · 2023-11-22T12:04:20Z

The code snippet will help you to understand how to leverage the inheritance to support multiple version:

(venv) lanfon@lanfonsuse:~/dev_src/tests> python
Python 3.10.2 (main, Jan 19 2022, 22:05:06) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from harvester_api.managers import HostManager as hm, ImageManager as im
>>> hm._sub_classes[hm]  # the attribute `_sub_classes` inherited from BaseManager
[<class 'harvester_api.managers.hosts.HostManager'>]
>>> im._sub_classes[im]
[<class 'harvester_api.managers.images.ImageManager'>]
>>> class H2(hm):
...   support_to = '1.1.0'
... 
>>> hm._sub_classes[hm]  # when new class derived, it would be added into the dict by `__init_sub_classes__`
[<class 'harvester_api.managers.hosts.HostManager'>, <class '__main__.H2'>]
>>> hm._sub_classes[H2]  # The list for derived class `H2` would also be added
[<class '__main__.H2'>]
>>> 
>>> class H3(H2):
...   support_to = '1.2.0'
... 
>>> hm._sub_classes[hm]
[<class 'harvester_api.managers.hosts.HostManager'>, <class '__main__.H2'>, <class '__main__.H3'>]
>>> hm._sub_classes[H2]   # when H3 inherited from H2, all its parents will have it
[<class '__main__.H2'>, <class '__main__.H3'>]
>>> hm.for_version('1.2.0')  # the `for_version` is going to the target version in the class' sub classes (by using `is_support`)
<class '__main__.H3'>
>>> H3.for_version('1.1.0')  # so if its sub class don't have any suitable version, we will return itself
<class '__main__.H3'>

In the api.py, we add new method load_managers(version) to adjust supported managers for api authorized cluster. (when use api.Login to do authorization, managers will be load with the cluster's version.

You can check #979 for the similar case (inheritance and seeking) be used in terraform provider version.

albinsun

LGTM

lanfon72 added 3 commits November 9, 2023 17:00

+ [apiclient] add type stub files

d592c3a

* [apiclient] reorganize models and managers

c29789e

* [apiclient] reorganize stub files

7149ad5

lanfon72 force-pushed the typing branch from 0a64e3a to 7149ad5 Compare November 9, 2023 17:17

* [apiclient] fix for_version inherit bug and add unit test

c6f065a

lanfon72 force-pushed the typing branch from 2611dda to c6f065a Compare November 13, 2023 15:38

lanfon72 requested a review from a team November 13, 2023 16:02

lanfon72 changed the title ~~restructure models and managers~~ [apiclient] restructure models and managers Nov 13, 2023

albinsun requested changes Nov 15, 2023

View reviewed changes

* [apiclient] temporarily fix for python3.6

1284dad

lanfon72 requested a review from albinsun November 15, 2023 16:27

albinsun requested a review from a team November 21, 2023 02:07

albinsun reviewed Nov 22, 2023

View reviewed changes

+ [apiclient] add docstring for BaseManager

1dc9987

albinsun approved these changes Nov 23, 2023

View reviewed changes

albinsun requested a review from a team November 23, 2023 00:57

lanfon72 merged commit db1a87e into harvester:main Nov 23, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[apiclient] restructure models and managers #982

[apiclient] restructure models and managers #982

lanfon72 commented Nov 9, 2023 •

edited

Loading

albinsun commented Nov 14, 2023 •

edited

Loading

lanfon72 commented Nov 14, 2023

albinsun left a comment

lanfon72 commented Nov 15, 2023 •

edited

Loading

albinsun commented Nov 18, 2023

lanfon72 commented Nov 18, 2023

albinsun left a comment

albinsun Nov 22, 2023 •

edited

Loading

lanfon72 Nov 22, 2023 •

edited

Loading

lanfon72 commented Nov 22, 2023 •

edited

Loading

albinsun left a comment

[apiclient] restructure models and managers #982

[apiclient] restructure models and managers #982

Conversation

lanfon72 commented Nov 9, 2023 • edited Loading

Changes

Footnotes

albinsun commented Nov 14, 2023 • edited Loading

lanfon72 commented Nov 14, 2023

albinsun left a comment

Choose a reason for hiding this comment

lanfon72 commented Nov 15, 2023 • edited Loading

albinsun commented Nov 18, 2023

411

412

lanfon72 commented Nov 18, 2023

albinsun left a comment

Choose a reason for hiding this comment

albinsun Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

lanfon72 Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

lanfon72 commented Nov 22, 2023 • edited Loading

albinsun left a comment

Choose a reason for hiding this comment

lanfon72 commented Nov 9, 2023 •

edited

Loading

albinsun commented Nov 14, 2023 •

edited

Loading

lanfon72 commented Nov 15, 2023 •

edited

Loading

albinsun Nov 22, 2023 •

edited

Loading

lanfon72 Nov 22, 2023 •

edited

Loading

lanfon72 commented Nov 22, 2023 •

edited

Loading