Conversation
jnsgruk
left a comment
jnsgruk
left a comment
There was a problem hiding this comment.
Overall this is looking great, thanks @yi-portainer!
A couple of possible improvements:
| [project] | ||
| name = "portainer-charm" | ||
| description = "Kubernetes charm for Portainer" | ||
| license = "Apache 2.0" | ||
| python = "^3.8" | ||
| readme = "README.md" | ||
| homepage = "https://charmhub.io/portainer" | ||
| repository = "https://github.com/portainer/portainer-charm" | ||
| documentation = "https://charmhub.io/portainer/docs" | ||
|
|
There was a problem hiding this comment.
I think this can be dropped to avoid duplicating data from metadata.yaml that might go out of sync over time :)
| [project] | |
| name = "portainer-charm" | |
| description = "Kubernetes charm for Portainer" | |
| license = "Apache 2.0" | |
| python = "^3.8" | |
| readme = "README.md" | |
| homepage = "https://charmhub.io/portainer" | |
| repository = "https://github.com/portainer/portainer-charm" | |
| documentation = "https://charmhub.io/portainer/docs" |
|
|
||
| # Ignore libraries that do not have type hint nor stubs | ||
| [[tool.mypy.overrides]] | ||
| module = ["ops.*", "kubernetes.*", "flatten_json.*"] |
There was a problem hiding this comment.
| module = ["ops.*", "kubernetes.*", "flatten_json.*"] | |
| module = ["ops.*", "kubernetes.*"] |
I can't see this module actually being used anywhere.
| git+https://github.com/canonical/operator/#egg=ops | ||
| PyYAML | ||
| kubernetes | ||
| flatten_json |
There was a problem hiding this comment.
| flatten_json |
I can't see this module actually being used anywhere.
| self.unit.status = WaitingStatus('waiting for service account perconditions') | ||
| K8sService.create_k8s_service_by_config(self._app_name, self._config) | ||
| if not K8sService.create_k8s_service_account(self._app_name): | ||
| self.unit.status = WaitingStatus("waiting for service account perconditions") |
There was a problem hiding this comment.
| self.unit.status = WaitingStatus("waiting for service account perconditions") | |
| self.unit.status = WaitingStatus("waiting for service account preconditions") |
| K8sService.create_k8s_service_by_config(self._app_name, self._config) | ||
| if not K8sService.create_k8s_service_account(self._app_name): | ||
| self.unit.status = WaitingStatus("waiting for service account perconditions") | ||
| logger.info("waiting for service account perconditions, installation deferred") |
There was a problem hiding this comment.
| logger.info("waiting for service account perconditions, installation deferred") | |
| logger.info("waiting for service account preconditions, installation deferred") |
| @classmethod | ||
| def create_k8s_service_by_config(cls, app_name: str, config: PortainerConfig): | ||
| """Delete then create k8s service by stored config.""" | ||
| api = kubernetes.client.CoreV1Api(kubernetes.client.ApiClient()) |
There was a problem hiding this comment.
Just as an aside, we've been trialling lightkube in some of our charms. It's a lot simpler than the official kubernetes library, and has better support for newer Kubernetes versions. Specifically, it makes patching things a little easier: example
From my experience so far, it requires fairly minimal rework, and might result in some terser code, but no pressure either way! :)
| api = kubernetes.client.CoreV1Api(kubernetes.client.ApiClient()) | ||
|
|
||
| try: | ||
| api.list_namespaced_service(namespace=cls.get_namespace()) |
There was a problem hiding this comment.
You may wish to try a more privileged action like listing ClusterRoles or similar to try and get a better picture of whether the charm has sufficient permissions :)
| spec = utils.clean_nones( | ||
| client.sanitize_for_serialization(cls.build_k8s_spec_by_config(app_name, config)) | ||
| ) | ||
| body = [] | ||
| for k, v in spec.items(): | ||
| body.append( | ||
| { | ||
| "op": "replace", | ||
| "path": f"/spec/{k}", | ||
| "value": v, | ||
| } | ||
| ) | ||
| logger.debug(f"patching with body: {body}") | ||
| if body: | ||
| api.patch_namespaced_service( | ||
| name=app_name, | ||
| namespace=cls.get_namespace(), | ||
| body=body, | ||
| ) | ||
| else: | ||
| logger.info("nothing to patch, skip patching") | ||
| return |
There was a problem hiding this comment.
This method specifically might be better served with something like the Client.replace method in lightkube
| svc = container.get_services().get(cls.PEBBLE_SERVICE, None) | ||
| # Check if the service is already running | ||
| if not svc: |
There was a problem hiding this comment.
| svc = container.get_services().get(cls.PEBBLE_SERVICE, None) | |
| # Check if the service is already running | |
| if not svc: | |
| if not container.get_services().get(cls.PEBBLE_SERVICE, None): |
| return False | ||
|
|
||
| @classmethod | ||
| def _build_layer_by_config(cls, config: PortainerConfig) -> dict: |
There was a problem hiding this comment.
| def _build_layer_by_config(cls, config: PortainerConfig) -> dict: | |
| def _pebble_layer(cls, config: PortainerConfig) -> dict: |
2 participants
No description provided.