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

feat(hardware-listing): add non compatibles platforms #883

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

WilsonNet
Copy link
Collaborator

@WilsonNet WilsonNet commented Feb 5, 2025

Description

How to test

Go to the hardware listing page and try to enter in a hardware details page that has the same repeat the process.

Copy link
Collaborator

@MarceloRobert MarceloRobert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be working well but I made some comments in the code

backend/kernelCI_app/views/hardwareView.py Show resolved Hide resolved
backend/kernelCI_app/views/hardwareView.py Outdated Show resolved Hide resolved
backend/kernelCI_app/views/hardwareView.py Show resolved Hide resolved
backend/kernelCI_app/views/hardwareView.py Outdated Show resolved Hide resolved
backend/kernelCI_app/views/hardwareView.py Outdated Show resolved Hide resolved
Copy link
Contributor

@murilx murilx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have cases where the same compatible can have mulitple platforms (this does not matter the other way around since environment_compatible has priority over platform when creating a hardware details page). One example is amlogic,a311d (localhost). Shouldn't we account for this case? Maybe add a tooltip in the hardware listing page to show all of them or even just adding all of them to the cell with something like platforms.map(platform => <span>{platform}<br /></span> (in this case the backend should return all platforms in the response)

One other option would be to leave it like it is now and discuss this in the mid sprint meeting

Copy link
Contributor

@murilx murilx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to keep both tree and hardware listing consistent, the search filter in hardware listing should also filter the platforms column (like we can filter by commits in the tree listing for example)

@WilsonNet WilsonNet force-pushed the feat/hardware-listing branch 5 times, most recently from d3920cd to 6084a49 Compare February 7, 2025 16:03
Copy link
Collaborator

@MarceloRobert MarceloRobert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work

Copy link
Contributor

@murilx murilx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

# documentation purposes. This model is not used in the code.
# TODO Remove timestamp from the api and this model
class HardwareQueryParamsDocumentationOnly(BaseModel):
origin: str = Field(default=DEFAULT_ORIGIN)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I believe there's no need for Field(default=DEFAULT_ORIGIN) here. Only this should be sufficient

Suggested change
origin: str = Field(default=DEFAULT_ORIGIN)
origin: str = DEFAULT_ORIGIN



class HardwareQueryParams(BaseModel):
origin: str = Field(default=DEFAULT_ORIGIN)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto about Field

Comment on lines +168 to +171
query_params = HardwareQueryParams(
start_date=request.GET.get("startTimestampInSeconds"),
end_date=request.GET.get("endTimeStampInSeconds"),
origin=request.GET.get("origin"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's nice to see!

Copy link
Collaborator

@Francisco2002 Francisco2002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIce job!!

@WilsonNet WilsonNet force-pushed the feat/hardware-listing branch from 6084a49 to 11d95f3 Compare February 7, 2025 17:08
- List hardware that is not an environment_compatible but has platform
- Introduced `platform` field in `HardwareItem` TypedDict in
  `hardwareView.py`.
- Updated `_make_hardware_item` method to include `platform`.
- Modified `_get_results` method to process `platform` field.
- Added `platform` field to the frontend components:
  - `HardwareListingPage.tsx`
  - `HardwareTable.tsx`
  - `messages/index.ts`
  - `types/hardware.ts`

  Closes #867
@WilsonNet WilsonNet force-pushed the feat/hardware-listing branch from 11d95f3 to 80afbc2 Compare February 7, 2025 17:10
@WilsonNet WilsonNet merged commit 7bf4ddf into main Feb 7, 2025
5 checks passed
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.

Add platforms as option for hardware listing
4 participants