Skip to content

feat(models): add SnapInfo model for snapd response#908

Draft
PraaneshSelvaraj wants to merge 1 commit intocanonical:mainfrom
PraaneshSelvaraj:work/pydantic
Draft

feat(models): add SnapInfo model for snapd response#908
PraaneshSelvaraj wants to merge 1 commit intocanonical:mainfrom
PraaneshSelvaraj:work/pydantic

Conversation

@PraaneshSelvaraj
Copy link

WIP

Objective: introduce a Pydantic model for the snapd snap info response.


  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint && make test?

@PraaneshSelvaraj
Copy link
Author

Hi @bepri, raising this as a draft PR to confirm the model structure before proceeding with replacing the current dict-based access in get_host_snap_info.

The model currently includes only the fields consumed in snap_installer.py. Does this schema and placement look good, or would you prefer any adjustments before I continue wiring it in?

Copy link
Member

@bepri bepri left a comment

Choose a reason for hiding this comment

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

The implementation looks good so far, though I'm a bit on the fence about extra="allow" on the models. I think I'd like others' opinions on this though, because I could go either way. Thoughts, @lengau?

I think you should be good to proceed with the remaining work, regardless of the answer. Be sure to add some unit tests for this. I'd get a few real API responses from snapd, store them in variables in a test file, and then validate them. Then make sure it rejects an entirely malformed response and a response indicating an error from snapd.

@PraaneshSelvaraj
Copy link
Author

Hey, quick question about the test mocks.

In the implementation, the SnapInfo model currently only defines the fields we actively use in snap_installer.py (id, revision, publisher.id, and base). Some of the existing tests, though, mock a more complete snapd response like:

{
    "id": "",
    "name": "test-name",
    "type": "app",
    "base": "coreXX",
    "version": "0.1",
    "channel": "",
    "revision": "2",
    "publisher": {"id": ""},
    "confinement": "classic",
}

Since the model is defined with extra="allow", these additional fields will be accepted. Would you prefer that I keep those extra fields in the mocks as-is, or should I trim them down to just what the model defines?

or we can explicitly model more of the snapd response instead of relying on extra="allow"?

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.

2 participants