Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernelci.api.models: use StrictInt for version fields #2382

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

JenySadadia
Copy link
Collaborator

For int fields, pydantic internally converts fields to integer if possible e.g. request field value "2" is converted to 2 in the FastAPI request handler for model parsing. That's why KernelVersion.version is converted to int when received by API.
Instead of relying on implicit data type conversion for int field, use StrictInt to enforce defined data type in the request itself.

For `int` fields, pydantic internally converts fields
to integer if possible e.g. request field value `"2"` is
converted to `2` in the FastAPI request handler for
model parsing. That's why `KernelVersion.version` is
converted to `int` when received by API.
Instead of relying on implicit data type conversion
for `int` field, use `StrictInt` to enforce defined
data type in the request itself.

Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
@JenySadadia JenySadadia added the staging-skip Don't test automatically on staging.kernelci.org label Feb 15, 2024
@JenySadadia JenySadadia requested a review from r-c-n February 15, 2024 09:41
@JenySadadia JenySadadia removed the staging-skip Don't test automatically on staging.kernelci.org label Feb 16, 2024
@JenySadadia JenySadadia requested a review from r-c-n February 19, 2024 07:48
@nuclearcat nuclearcat added the staging-skip Don't test automatically on staging.kernelci.org label Feb 19, 2024
@nuclearcat
Copy link
Member

Causing exception during kci tools use on staging:

Build docker images: kernelci
Traceback (most recent call last):
  File "/home/nuclearcat/Documents/kci-easy/kernelci/kernelci-core/./kci", line 17, in <module>
    from kernelci.cli import kci  # noqa: E402
  File "/home/nuclearcat/Documents/kci-easy/kernelci/kernelci-core/kernelci/cli/__init__.py", line 26, in <module>
    import kernelci.api.helper
  File "/home/nuclearcat/Documents/kci-easy/kernelci/kernelci-core/kernelci/api/helper.py", line 14, in <module>
    from .models import KernelVersion
  File "/home/nuclearcat/Documents/kci-easy/kernelci/kernelci-core/kernelci/api/models.py", line 97, in <module>
    class Revision(BaseModel):
  File "/home/nuclearcat/Documents/kci-easy/kernelci/kernelci-core/kernelci/api/models.py", line 102, in Revision
    url: AnyUrl | FileUrl = Field(
TypeError: unsupported operand type(s) for |: 'type' and 'type'

@r-c-n
Copy link
Contributor

r-c-n commented Feb 19, 2024

@nuclearcat that's supposedly related to something in the staging environment using python < 3.10. @pawiecz and @JenySadadia know about this in more detail. Related: #2381

@pawiecz
Copy link
Contributor

pawiecz commented Feb 19, 2024

As @hardboprobot noted, this indeed looks like Py <= 3.10 issue.

@nuclearcat Please provide more details to track down this issue (e.g. command called, exec env - kci-easy?).

@nuclearcat
Copy link
Member

Yes, it can be reproduced also in latest kci-easy during build on debian bullseye.

@JenySadadia
Copy link
Collaborator Author

@nuclearcat that's supposedly related to something in the staging environment using python < 3.10. @pawiecz and @JenySadadia know about this in more detail. Related: #2381

| operator was introduced in python 3.10 and not available in 3.9
Please see fastapi/typer#371

@pawiecz
Copy link
Contributor

pawiecz commented Feb 19, 2024

| operator was introduced in python 3.10 and not available in 3.9

Right - I'm afraid importing models in helpers used by cli will be a no-go until we drop 3.9 entirely

@r-c-n
Copy link
Contributor

r-c-n commented Feb 19, 2024

Right - I'm afraid importing models in helpers used by cli will be a no-go until we drop 3.9 entirely

Ouch

@JenySadadia
Copy link
Collaborator Author

Right - I'm afraid importing models in helpers used by cli will be a no-go until we drop 3.9 entirely

True. Now, we are left with 2 options: one is type cast model fields in the tarball service but that is leaking model info. Another option is we can use KernelVersion.translate_version_fields in api.main.post_node handler itself to translate fields before parsing. We are using these kind of handlers for get_nodes atm.

Implement `KernelVersion.translate_version_fields` method to
translate version string fields to integers.

Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
@JenySadadia
Copy link
Collaborator Author

Right - I'm afraid importing models in helpers used by cli will be a no-go until we drop 3.9 entirely

True. Now, we are left with 2 options: one is type cast model fields in the tarball service but that is leaking model info. Another option is we can use KernelVersion.translate_version_fields in api.main.post_node handler itself to translate fields before parsing. We are using these kind of handlers for get_nodes atm.

See kernelci/kernelci-api#483

@JenySadadia JenySadadia requested a review from r-c-n February 20, 2024 04:53
@JenySadadia JenySadadia removed the staging-skip Don't test automatically on staging.kernelci.org label Feb 20, 2024
@JenySadadia JenySadadia added this pull request to the merge queue Feb 20, 2024
Merged via the queue into kernelci:main with commit e2fe015 Feb 20, 2024
4 checks passed
@JenySadadia JenySadadia deleted the use-strict-data-type branch February 20, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kver failure on staging
4 participants