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

Use model_validate built into pydantic to parse DB models #129

Merged
merged 3 commits into from
Sep 20, 2024

Conversation

b-rowan
Copy link
Contributor

@b-rowan b-rowan commented Sep 9, 2024

pydantic models have a deprecated from_orm method, which is designed to parse DB models into the schema. This was superseded by model_validate, which does the same thing when the schema config from_attributes value is true. Migrating to using this simplifies the method of parsing DB models, and will eventually allow the schemas to be used internally instead of the DB models.

Reference here - https://docs.pydantic.dev/2.0/usage/models/#arbitrary-class-instances

The goal here is to get to the point where we can pass around arbitrary abstract software types internally, which will allow hooking the software that a device should receive.

@UpstreamData UpstreamData force-pushed the dev_from_orm branch 3 times, most recently from 71a0270 to c66f338 Compare September 9, 2024 19:17
@b-rowan b-rowan requested a review from tsagadar September 13, 2024 15:43
goosebit/ui/bff/rollouts/responses.py Show resolved Hide resolved
goosebit/db/models.py Outdated Show resolved Hide resolved
@tsagadar
Copy link
Collaborator

tsagadar commented Sep 17, 2024

Took my third attempt to wrap my head around this PR - with limited success. I think there are three things in this PR

  1. Simplify the Pydantic model instantiation relying on from_attributes. As an independent commit I think it would be an obvious valuable refactoring.
  2. Move logic from the DB models (entities) to the schema (DTO). Not really sure if this is a good idea. Usually logic would remain in the entities.
  3. Preparation work for implementing the "initial update for device without unique identifier" use case through a plugin mechanism. It could help to describe this as a feature request - even if the idea is to implement it as a plug-in. This could allow to understand how this can fit into gooseBit. Maybe the feature should even be part of the regular code base and that can be activated through a configuration?

@b-rowan
Copy link
Contributor Author

b-rowan commented Sep 17, 2024

3. Preparation work for implementing the "initial update for device without unique identifier" use case through a plugin mechanism. It could help to describe this as a feature request - even if the idea is to implement it as a plug-in. This could allow to understand how this can fit into gooseBit. Maybe the feature should even be part of the regular code base and that can be activated through a configuration?

It's not specifically that, I actually had a cloudflare integration in mind. Right now with our internal version (which is very outdated), we have a cloudflare integration which creates tunnels to devices automatically based on UUIDs. This requires configuring cloudflared on the device with a specific private key, and in order to do that we are generating a custom SWU that only writes to that config file, then pushing it to the device. So the idea would be to check for gooseBit updates for a device, then check for custom SWU file from plugin, and to do that we need a standard internal interface that doesn't require creating a DB entry (as I would like to delete these SWU's off disk ASAP, we can always regenerate them later).

@UpstreamData UpstreamData force-pushed the dev_from_orm branch 2 times, most recently from a740559 to 42926d8 Compare September 17, 2024 16:03
@b-rowan
Copy link
Contributor Author

b-rowan commented Sep 17, 2024

Simplify the Pydantic model instantiation relying on from_attributes. As an independent commit I think it would be an obvious valuable refactoring.

I think you're right about this for now. I have made this PR a bit more atomic by removing 2 and 3. I would really like to be able to get a plugin interface of some kind included here, I want to have our internal stuff up to date, but it seems every foothold I've tried to gain on that side of things causes one issue or another. If you have any ideas on that side of things LMK.

@b-rowan b-rowan requested review from tsagadar and removed request for jameshilliard September 19, 2024 16:35
Copy link
Collaborator

@tsagadar tsagadar left a comment

Choose a reason for hiding this comment

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

Looks good. Just minor issues that are optional to address in my opinion.

goosebit/api/v1/software/routes.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
goosebit/api/v1/rollouts/routes.py Show resolved Hide resolved
Not needed anymore since aiofiles was removed.
When sending a rollout put request via the  API, there was no validation that the referenced software file existed.
`pydantic` models have a deprecated `from_orm` method, which is designed to parse DB models into the schema.
This was superseded by `model_validate`, which does the same thing when the schema config `from_attributes` value is true.
Migrating to using this simplifies the method of parsing DB models, and will eventually allow the schemas to be used internally instead of the DB models.
@b-rowan b-rowan merged commit b3654a8 into master Sep 20, 2024
5 checks passed
@b-rowan b-rowan deleted the dev_from_orm branch September 20, 2024 16:53
@tsagadar
Copy link
Collaborator

Simplify the Pydantic model instantiation relying on from_attributes. As an independent commit I think it would be an obvious valuable refactoring.

I think you're right about this for now. I have made this PR a bit more atomic by removing 2 and 3. I would really like to be able to get a plugin interface of some kind included here, I want to have our internal stuff up to date, but it seems every foothold I've tried to gain on that side of things causes one issue or another. If you have any ideas on that side of things LMK.

@b-rowan let's discuss this through another channel. Can you send me an email to the address from my commits?

@b-rowan
Copy link
Contributor Author

b-rowan commented Sep 24, 2024

Can you send me an email to the address from my commits?

Done. Sent from my upstreamdata.ca email.

@@ -1,3 +1,5 @@
from __future__ import annotations
Copy link
Collaborator

@easybe easybe Sep 30, 2024

Choose a reason for hiding this comment

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

@b-rowan why are these lines needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure why I've been adding them, just a good failsafe. Need to modify my IDE settings to stop complaining about them I think.

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.

3 participants