Skip to content

FIX BED-8688: Make Policy model validation requirements less strict#8

Merged
d3vzer0 merged 6 commits into
mainfrom
fix/BED-8688
Jun 22, 2026
Merged

FIX BED-8688: Make Policy model validation requirements less strict#8
d3vzer0 merged 6 commits into
mainfrom
fix/BED-8688

Conversation

@d3vzer0

@d3vzer0 d3vzer0 commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Added several (Policy) model changes to fix/prevent issues for the following:

  • retry_event: Model validation fails when the retry_event field contains a value not defined in the Retry enum. This change removes the Enum and accepts any (string) value.
  • user_groups: It seems that the JAMF API can return user_groups in different formats. Added a secondary format to accept both a list containing a key/value dict or a list containing simple strings.
  • Various model changes: Modified several models to not interpret field values as type Any but force the value to be read (and stored) as a string. This does not fix an existing issue, but may prevent future issues where the API response may return multiple types for the same field, which may cause the pipeline to fail.

@d3vzer0 d3vzer0 marked this pull request as draft June 18, 2026 07:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Jamf Policy Pydantic models to be more tolerant of Jamf API response shape/typing variability, preventing validation failures in the collection pipeline.

Changes:

  • Removes strict enums for trigger and retry_event, accepting any string value.
  • Broadens model fields intended to handle inconsistent Jamf API formats (e.g., user group representations).
  • Refactors several nested structures (e.g., self service categories, packages, dock items) into dedicated models / stricter annotations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/openhound_jamf/models/policy.py Outdated
Comment thread src/openhound_jamf/models/policy.py Outdated
Comment thread src/openhound_jamf/models/policy.py
Comment thread src/openhound_jamf/models/policy.py Outdated
Comment thread src/openhound_jamf/models/policy.py Outdated
Comment thread src/openhound_jamf/models/policy.py Outdated
Comment thread src/openhound_jamf/models/policy.py Outdated
Comment thread src/openhound_jamf/models/policy.py Outdated
Comment thread src/openhound_jamf/models/policy.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comment thread src/openhound_jamf/models/policy.py
Comment thread src/openhound_jamf/models/policy.py
@d3vzer0 d3vzer0 marked this pull request as ready for review June 18, 2026 08:30

@StranDutton StranDutton left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The enum removal for trigger/retry_event makes total sense and I think that's the right call.

I had a question about the dict[str, Any] -> dict[str, IdName] changes across the fields. I traced through the pipeline to understand how these models interact with DLT and wanted to flag something.

DLT will currently reject rows where a field's type doesn't match the schema in the model (due to Contract.freeze in app.py). I don't think we have experienced that behavior yet because Any is very permissive and wouldn't cause a type conflict even if the API response contained fields we hadn't seen before, since it just stores the response as it is received.

A few fields in the current tests would get silently dropped as a result of these changes too. For example, exclusions.network_segments has a uid field that would be silently dropped. If this is known and intentional, I'm totally open to discuss, but I just wanted to confirm first!

If the goal right now is to handle API response variability and be less strict, I think it would be best to leave dict[str, IdName] as dict[str, Any] for now, as we can always go back and tighten up the models in the future. I just think the model changes should be tracked with their own ticket/PR.

TL;DR: I am good with most of the changes made to fix the bug, but have concerns about the dict[str, IdName] model changes

EDIT: re-evaluating after seeing the proposed changed to DLT contract

@StranDutton StranDutton dismissed their stale review June 22, 2026 18:15

I am going to take back these recommendations for now and we can re-address later! The concern I brought up is more about silent data loss, and after looking into it some more, I am good to approve this as it stands since it does fix the bug

@d3vzer0 d3vzer0 merged commit b9b0135 into main Jun 22, 2026
2 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.

4 participants