Skip to content

Conversation

@mirkoCrobu
Copy link
Contributor

Motivation

closes #114

The v1/apps/{id}/bricks response must not include compatible_models fields.

Change description

Decouple the v1/apps/{id}/bricks and v1/apps/{id}/bricks/{brick_id} response structures by creating a dedicated struct for the v1/apps/{id}/bricks endpoint.

Additional Notes

Reviewer checklist

  • PR addresses a single concern.
  • PR title and description are properly filled.
  • Changes will be merged in main.
  • Changes are covered by tests.
  • Logging is meaningful in case of troubleshooting.

@mirkoCrobu mirkoCrobu requested a review from dido18 November 27, 2025 12:08
@mirkoCrobu mirkoCrobu self-assigned this Nov 27, 2025
@mirkoCrobu mirkoCrobu force-pushed the issue_114_rm_compatible_modesl_from_instance_brick_list branch from 8ae6e81 to dccfa09 Compare November 27, 2025 15:28
Copy link
Contributor

@dido18 dido18 left a comment

Choose a reason for hiding this comment

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

Don't we have tests for the endpoint or bricks ?
I see no modification in *_test.go files

@mirkoCrobu
Copy link
Contributor Author

Don't we have tests for the endpoint or bricks? I see no modification in *_test.go files

We already have both unit and e2e tests for TestBricksDetails, added in the previous issues handled by Giulio and me.
Those tests cover all the expected fields, and I didn’t need to update them since this PR is removing a field that shouldn’t be there.
The field likely appeared due to a bad rebase or merge(?).

Author: "Arduino", // TODO: for now we only support our bricks
Category: brick.Category,
Status: "installed",
RequireModel: brick.RequireModel,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the require_model must be kept in the response, isn't it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I preferred to restart the PR here: #120
Ensuring to have an up-to-date repo and integrating unit and e2e tests.

@mirkoCrobu
Copy link
Contributor Author

moved #120

@mirkoCrobu mirkoCrobu closed this Nov 27, 2025
@per1234 per1234 added the duplicate This issue or pull request already exists label Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the compatible_models from the /v1/apps/{id}/bricks

3 participants