Conversation
mandopaloooza
force-pushed
the
replace-typedload
branch
from
54ffe40 to
be519a3
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces the typedload-based entity (de)serialization implementation with cattrs, updates Python dependency pins accordingly, and bumps the Helm chart/app versions to reflect the change.
Changes:
- Replace
typedloadloader/dumper logic withcattrs.Converter-based structuring/unstructuring inappgate/attrs.py. - Update dependency inputs/locks to remove
typedloadand addcattrs. - Bump Helm chart
versionandappVersionfrom0.5.0to0.5.1for both CRD and operator charts.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| requirements.in | Swap typedload for cattrs as a direct dependency. |
| requirements.txt | Lock cattrs and remove typedload from the compiled requirements. |
| k8s/operator/Chart.yaml | Bump chart/app versions to 0.5.1. |
| k8s/crd/Chart.yaml | Bump chart/app versions to 0.5.1. |
| appgate/attrs.py | Rework entity dump/load to use cattrs and custom hooks/exceptions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AppgateLoadException = getattr( | ||
| openapi_types, | ||
| "Appgate" + "Typed" + "loadException", | ||
| ) |
There was a problem hiding this comment.
AppgateLoadException is being obtained via getattr(openapi_types, "Appgate" + "Typed" + "loadException"), which hides the dependency from static analysis/type checking and will fail at import time with an AttributeError if the symbol ever changes. Prefer a direct import/alias (e.g., import AppgateTypedloadException as AppgateLoadException) so linters and type checkers can validate it and refactors are safer.
| def parse_datetime(value: Any) -> datetime.datetime: | ||
| try: | ||
| # dateutil.fromisofromat cannot handle string that contains 6+ sub-second digits | ||
| # dateutil.fromisofromat cannot handle strings that contain 6+ sub-second digits |
There was a problem hiding this comment.
The comment mentions dateutil.fromisofromat, which is misspelled and also not the API being used here (parser.isoparse). Please correct the wording to avoid confusion (likely referring to datetime.fromisoformat’s limitations with >6 fractional digits).
| # dateutil.fromisofromat cannot handle strings that contain 6+ sub-second digits | |
| # Use dateutil.parser.isoparse because datetime.fromisoformat cannot | |
| # handle strings that contain more than 6 sub-second digits. |
| elif strict: | ||
| raise AppgateTypedloadException( | ||
| raise AppgateLoadException( | ||
| "Unable to dump entity: name/id field is missing", | ||
| platform_type=PlatformType.K8S, | ||
| ) |
There was a problem hiding this comment.
k8s_dumper is used for both K8S and GIT dumping (see get_dumper), but the exception hard-codes platform_type=PlatformType.K8S. This will mislabel errors raised while dumping for the Git platform. Consider passing the caller’s platform_type into k8s_dumper (or selecting the appropriate PlatformType when raising).
| dumper: Callable[ | ||
| [Entity_T, bool, Dict[str, List[MissingFieldDependencies]] | None], | ||
| Dict[str, Any], | ||
| ] | ||
|
|
||
| if platform_type in {PlatformType.K8S, PlatformType.GIT}: |
There was a problem hiding this comment.
When api_spec is missing for PlatformType.GIT, this raises AppgateLoadException(..., platform_type=PlatformType.K8S), which reports the wrong platform in logs/error handling. Use the actual platform_type value (K8S vs GIT) when constructing the exception.
| if (read_only and platform_type == PlatformType.K8S) or ( | ||
| write_only | ||
| and platform_type | ||
| in { | ||
| PlatformType.APPGATE, | ||
| PlatformType.GIT, | ||
| } | ||
| write_only and platform_type in {PlatformType.APPGATE, PlatformType.GIT} | ||
| ): | ||
| # Don't load attribute from K8S in read only mode even if | ||
| # Don't load attribute from K8S in read-only mode even if |
There was a problem hiding this comment.
This branch is dead code (pass) and can be removed. If it’s meant to enforce/track required fields, implement that logic (or rely on structure_attrs_fromdict raising a clear error) rather than leaving an empty conditional.
Description
Checklist
.versionand.appVersionin k8s/crd/Chart.yaml.versionand.appVersionin k8s/operator/Chart.yaml