-
Notifications
You must be signed in to change notification settings - Fork 19
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
Feat/upgrade pydantic v2 #354
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments
src/charm_state.py
Outdated
@@ -407,12 +403,12 @@ class CharmConfig(BaseModel): | |||
""" | |||
|
|||
denylist: list[FirewallEntry] | |||
dockerhub_mirror: AnyHttpsUrl | None | |||
dockerhub_mirror: AnyHttpsUrl | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to add a default value?
src/charm_state.py
Outdated
labels: tuple[str, ...] | ||
openstack_clouds_yaml: OpenStackCloudsYAML | None | ||
openstack_clouds_yaml: OpenStackCloudsYAML | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to add a default value?
src/charm_state.py
Outdated
path: GitHubPath | ||
reconcile_interval: int | ||
repo_policy_compliance: RepoPolicyComplianceConfig | None | ||
repo_policy_compliance: RepoPolicyComplianceConfig | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to add a default value?
@@ -486,8 +483,6 @@ def _parse_openstack_clouds_config(cls, charm: CharmBase) -> OpenStackCloudsYAML | |||
openstack_clouds_yaml: OpenStackCloudsYAML = yaml.safe_load( | |||
cast(str, openstack_clouds_yaml_str) | |||
) | |||
# use Pydantic to validate TypedDict. | |||
create_model_from_typeddict(OpenStackCloudsYAML)(**openstack_clouds_yaml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IINM, this raises if the input dict does not match the typedict, so there is a change in functionality. I see in pydantic 2 something similar https://docs.pydantic.dev/2.3/usage/types/dicts_mapping/#typeddict (not sure if appropriate as it says that for python < 3.12 you may need a library)
src/charm.py
Outdated
@@ -1182,8 +1182,9 @@ def _on_image_relation_joined(self, _: ops.RelationJoinedEvent) -> None: | |||
return | |||
|
|||
clouds_yaml = state.charm_config.openstack_clouds_yaml | |||
cloud = list(clouds_yaml["clouds"].keys())[0] | |||
auth_map = clouds_yaml["clouds"][cloud]["auth"] | |||
# unsubscriptable-object / thinks cloud_yaml is kind of a list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at charm_config it is OpenStackCloudsYAML | None
, so it probably thinks it can be None, which it can be....
src/charm_state.py
Outdated
@@ -496,15 +492,15 @@ def _parse_openstack_clouds_config(cls, charm: CharmBase) -> OpenStackCloudsYAML | |||
|
|||
try: | |||
openstack_cloud.initialize(openstack_clouds_yaml) | |||
except OpenStackInvalidConfigError as exc: | |||
except (OpenStackInvalidConfigError, TypeError) as exc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the linter has found something strange after the change in:
openstack_clouds_yaml: OpenStackCloudsYAML | None
openstack_clouds_yaml: OpenStackCloudsYAML | None = None
src/charm_state.py
Outdated
if not proxy_address.port | ||
else f"{proxy_address.host}:{proxy_address.port}" | ||
) | ||
if "http" in proxy_address.host: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is http or https in the host?
@@ -194,7 +194,7 @@ def create_runner(self, registration_token: str) -> InstanceId: | |||
instance_id=instance_id, | |||
image=self._server_config.image, | |||
flavor=self._server_config.flavor, | |||
network=self._server_config.network, | |||
network=str(self._server_config.network) if self._server_config.network else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testing locally I do not need the change.
…rator into feat/upgrade-pydantic-v2
Test coverage for 368b7e6
Static code analysis report
|
Updated Pydantic version to v2.8.2
Overview
Rationale
Juju Events Changes
Module Changes
Library Changes
Checklist
src-docs
urgent
,trivial
,complex
)