Charm Review #1
Charm Review #1
Conversation
…n-hydra fix: check if traefik is ready instead of checking if relation is created
Fix workflows
Fix return types, check leadership, convert SUPPORTED_SCOPES to list
Update dashboards and alert rules
Fix database/migration logic
docs: IAM-410 add charmhub doc link
…actions chore(deps): update actions/checkout digest to b4ffde6
…badge Added charmhub badge to README.md
Enabling the auto-merge option in the create-pull-request action tried to squash the commits, which is not allowe dby our repo config.
Fix publish libs action
…-query fix(loki-rule): improve error handling in json parsing
…m-libs (Automated) Update Charm Libs
- Change the _on_relation_changed_event to ensure that all units receive the hook. Currently only leader units on the consuming charm will receive the event which is not correct as non-leader units also need the info.
Update _on_relation_changed_event
…version Bump hydra to 2.2.0
chore: add icon.svg
| from dataclasses import asdict, dataclass, field | ||
| from typing import Dict, List, Mapping, Optional | ||
|
|
||
| import jsonschema |
There was a problem hiding this comment.
Probably a good idea to add jsonschema to PYDEPS so that charmcraft will automatically install it for user charms. See https://discourse.charmhub.io/t/python-dependencies-in-charm-libraries/8599
| parts: | ||
| charm: | ||
| charm-binary-python-packages: | ||
| - jsonschema |
There was a problem hiding this comment.
I tested with the latest version of charmcraft, and it can build jsonschema from the source without any problems, so this can probably be removed.
| @classmethod | ||
| def from_dict(cls, dic: Dict) -> "OauthProviderConfig": | ||
| """Generate OauthProviderConfig instance from dict.""" | ||
| return cls(**{k: v for k, v in dic.items() if k in inspect.signature(cls).parameters}) |
There was a problem hiding this comment.
Perhaps the dataclasses.fields function will be simpler than inspect.signature?
|
|
||
| def restore(self, snapshot: Dict) -> None: | ||
| """Restore event.""" | ||
| self.client_id = snapshot["client_id"] |
There was a problem hiding this comment.
| self.client_id = snapshot["client_id"] | |
| super().restore(snapshot) | |
| self.client_id = snapshot["client_id"] |
Probably should call the overridden restore function.
| and "client_secret_id" in relation.data[relation.app] | ||
| ) | ||
|
|
||
| def get_provider_info(self, relation_id: Optional[int] = None) -> OauthProviderConfig: |
There was a problem hiding this comment.
| def get_provider_info(self, relation_id: Optional[int] = None) -> OauthProviderConfig: | |
| def get_provider_info(self, relation_id: Optional[int] = None) -> Optional[OauthProviderConfig]: |
small nit
| "version": { | ||
| "override": "replace", | ||
| "exec": {"command": "hydra version"}, | ||
| }, |
There was a problem hiding this comment.
Is this necessary, since it only checks whether the container has the hydra executable and not whether the hydra service is running?
| try: | ||
| self._container.get_service(self._container_name) | ||
| except (ModelError, RuntimeError): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
| try: | |
| self._container.get_service(self._container_name) | |
| except (ModelError, RuntimeError): | |
| return False | |
| return True | |
| return self._container.can_connect() and self._container_name in self._container.get_services() |
So we don't need to capture the RuntimeError which may caused by some unknown problems.
| def _get_database_relation_info(self) -> dict: | ||
| if not self.database.relations: | ||
| return None | ||
|
|
||
| relation_id = self.database.relations[0].id | ||
| relation_data = self.database.fetch_relation_data()[relation_id] | ||
| return { | ||
| "username": relation_data.get("username"), | ||
| "password": relation_data.get("password"), | ||
| "endpoints": relation_data.get("endpoints"), | ||
| "database_name": self._db_name, | ||
| } |
There was a problem hiding this comment.
It would probably be better to check if the relation data contains the username, password, and endpoints fields, as the PostgreSQL charm may take some time to react and provision the database. During that time, the relation exists, but the relation data will be empty. Also, the endpoints field is a comma-separated list of all possible endpoints (e.g., 10.0.0.1:5432,10.0.0.2:5432). Does Hydra support multiple host components within the URI?
| def _on_database_relation_departed(self, event: RelationDepartedEvent) -> None: | ||
| """Event Handler for database relation departed event.""" | ||
| self.unit.status = BlockedStatus("Missing required relation with postgresql") |
There was a problem hiding this comment.
As mentioned before, I think maybe relation-broken might be a better choice here as if the database scale down an unit, it will trigger the relation-departed event.
| if not self._container.can_connect(): | ||
| return False | ||
|
|
||
| try: | ||
| service = self._container.get_service(self._container_name) | ||
| except (ModelError, RuntimeError): | ||
| return False |
No description provided.