feat: Implements route53 lego k8s operator (do not merge) #38
Conversation
| def _on_certificate_creation_request(self, event: CertificateCreationRequestEvent) -> None: | ||
| """Handles certificate creation request event. | ||
|
|
||
| - Retrieves subject from CSR |
There was a problem hiding this comment.
Nitpick: Instead of commenting about what this event callback method does, I would say it would be better to make the code self-describe what it does. This method is kinda long so it can be decomposed to several small functions.
There was a problem hiding this comment.
The charm here is not an owner of this lib + the method is ~20 lines long and it already calls those "small functions". In any case, this is not a charm review criteria.
| def _pull_certificates_from_workload(self, csr_subject: str) -> List[Union[bytes, str]]: | ||
| """Pulls certificates from workload container.""" | ||
| chain_pem = self._container.pull(path=f"{self._certs_path}{csr_subject}.crt") | ||
| return [cert for cert in chain_pem.read().split("\n\n")] # type: ignore[arg-type] |
There was a problem hiding this comment.
Nitpick: I am curious why this method returns a list and the caller has to use the index to get the certificate, ca, and chain. It would be better to directly make the return data ready.
Another question is: is the order guaranteed to be the same everytime?
There was a problem hiding this comment.
Again, this charm here is not a owner of this lib, we may want to change this but that does not feel like a charm review criteria
| isort {[vars]all_path} | ||
| black {[vars]all_path} | ||
|
|
||
| [testenv:lint] |
There was a problem hiding this comment.
Nitpick: should we switch to ruff?
There was a problem hiding this comment.
Maybe in the future, but this is not a charm review criteria
| raise_on_blocked=True, | ||
| timeout=1000, | ||
| ) | ||
| assert ops_test.model.applications[APP_NAME].units[0].workload_status == "active" |
There was a problem hiding this comment.
Is this still needed when we already wait for status to be active above?
There was a problem hiding this comment.
Will be addressed by PR #50
|
|
||
|
|
||
| @pytest.mark.abort_on_fail | ||
| async def test_build_and_deploy(ops_test): |
There was a problem hiding this comment.
Besides testing build and deployment, is it possible that we can also test the integration with a requirer charm?
There was a problem hiding this comment.
Will be addressed by PR #50
Description
The goal of this PR is to help in the process of having this charm listed in Charmhub following the charm review process. Here is the link to the charm review request.
Please do not merge this PR.
Checklist: